💬 ชาวเน็ตแห่แชร์: openzfs/zfs/pull/

tonyhutter · 2019-09-21T00:24:16Z

Motivation and Context

Add colored zpool status output so admins can quickly spot a unhealthy vdev/pool.

Description

If the ZFS_COLOR env variable is set, then use ANSI color output in zpool status:

Column headers are bold
Degraded or offline pools/vdevs are yellow
Non-zero error counters and faulted vdevs/pools are red
The 'status:' and 'action:' sections are yellow if they're displaying a warning.

Screenshots:

How Has This Been Tested?

Manually tested. Added test case to look for bold, yellow and red text.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

tonyhutter · 2019-10-22T23:57:22Z

Looks like I'm seeing some test failures:

Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool/zpool_colors (run as root) [00:04] [FAIL]
20:54:29.63 ASSERTION: Test colorized zpool status output
20:54:29.70 SUCCESS: dd if=/dev/urandom of=//mnt/testdir/testfile bs=10M count=1
20:54:30.56 SUCCESS: zpool export testpool
20:54:31.78 SUCCESS: zpool import testpool
20:54:31.78 'unknown': I need something more specific.
20:54:31.78 NOTE:   
20:54:31.82 SUCCESS: zpool offline -f testpool loop4

This is coming from ncurses:

'unknown': I need something more specific.

Maybe we need ncurses-term installed on Ubuntu?:

https://unix.stackexchange.com/questions/213726/ssh-from-screen-leads-to-unknown-terminal-error

I'm looking into it.

ahrens · 2019-11-13T18:20:39Z

Live demo from the 2018 OZDS hackathon, where this project won first place 🥇 : https://youtu.be/zN_tGxCpTBU?t=430

When do we get 🔥 emoji support? :-)

ahrens · 2019-11-13T18:21:58Z

Would it be possible to auto-detect if the terminal supports color and turn this on by default, like git seems to do?

tonyhutter · 2019-11-14T01:31:36Z

@ahrens one issue is that people screen scrape the zpool status output with awk. Each color escape code counts as a field in awk, so if we turned on color by default it's probably going to mess up the column counting in a bunch of scripts.

I decided to pass on the emojis for now, since I didn't know of a good way to detect if they're supported in the terminal. And they're a little too silly :-)

lundman · 2019-11-14T01:55:10Z

Generally, things like git use isatty() to have colour, and disable if not, so if you pipe to awk it will be disabled.

spacelama · 2019-11-14T03:58:22Z

And --color[=yes|auto|no (default yes if --color supplied without a parameter)], defaulting to
--color=auto if --color not supplied.

tonyhutter · 2019-11-14T16:47:26Z

@lundman thanks for the suggestion about isatty(). Let me look into that. I'd personally like the colors on by default if we know that it's not going to interfere with anything.

richardelling · 2019-11-15T14:06:02Z

tty colors are a human factors disaster. theee is no way for the dev to know the contrast of the terminal’s colors. so the dev says “I think good things should be green” without realizing that some folks use green as the background, rendering it unreadable. the typical egregious colors are blue/black, but the real problem is you have no idea what the users preferences are. also cut-n-paste often picks up the background and foreground colors... spreading the ugliness. personally, I use different color backgrounds to differentiate being logged into different machines, workflows, or tasks. but the most telling story is recently a colleague did this in a product, when we went onsite with a customer, the customer’s preferences rendered more than 50% of the text unreadable. thus the product had to be changed to allow the customer to choose color combinations (of which there are thousands to choose from) for each and every possible rendering. human factors disaster compounded.

just say no to colors by default

behlendorf · 2019-11-20T22:15:41Z

+#include <term.h>
+/*
+ * For some reason, term.h #defines 'lines' (in lower case) as a macro.
+ * Since we also have a variable named 'lines', undefine the macro here.


Even though this works, I think we should go ahead and rename the our local lines variable to avoid the conflict entirely. Then drop this block. According to the curses documentation, curs_terminfo(3X).

· If use_env(FALSE) has been called, values for lines and columns specified in terminfo are used.

Which would be handy if we decide it's desirable to use more of the libraries functionality.

behlendorf · 2019-11-20T22:22:12Z

+		 * If it's not supported, then silently don't use it.
+		 */
+		if (setupterm(NULL, fileno(stdout), (int *)0) == 0 &&
+		    tigetnum("colors") >= 16) {


Maybe && isatty(stdout) for good measure.

behlendorf · 2019-11-20T22:26:33Z

+		color_start(color);
+
+	va_start(aptr, format);
+	vsprintf(buf, format, aptr);


Can you use vsnprintf() here to avoid any overflow and make sure it's null-terminated.

behlendorf · 2019-11-20T22:29:22Z

+Use ANSI color in
+.Nm zpool status
+output.
+.El


Can you move this up after ZFS_ABORT to keep the environment variables alphabetically ordered. Moving ZFS_VDEV_DEVID_OPT_OUT` up wouldn't be a bad thing either.

tonyhutter · 2019-11-22T01:15:29Z

It sounds like there's no consensus on colors on/off by default, so we'll keep them off unless you export ZFS_COLOR=1.

@behlendorf I implemented all your changes in my latest push. I'm still getting test failures in buildbot though, so I've added some instrumentation printfs. Please don't pull this in until I can get all the tests passing.

tonyhutter · 2019-12-12T16:41:12Z

@behlendorf my latest push fixes all your comments

behlendorf · 2019-12-18T02:10:00Z

@lundman @freqlabs would you mind double checking this PR to ensure it does make sense for your respective platforms. We can f course tweak it latter if needed. By default, the colors are disabled so you will need to set the ZFS_COLOR environment variable if you test it locally.

lundman · 2019-12-18T02:18:26Z

PR to ensure it does make sense for your respective platforms.

Looks to be standard porting, strcmp -> strlcmp for the noise reduction (deprecated) , not sure about exit() in a library, but if you are OK with it, so am I.
Not sure what comment "FG" for forground color refers to, but then... it's 'just a comment' :)

Yeah, should be fine to port over.

tonyhutter · 2019-12-18T20:49:11Z

@lundman @freqlabs thanks, I'll implement your changes.

codecov · 2019-12-19T07:58:50Z

Codecov Report

Merging #9340 into master will decrease coverage by <1%.
The diff coverage is 66%.

@@           Coverage Diff            @@
##           master    #9340    +/-   ##
========================================
- Coverage      80%      79%   -<1%     
========================================
  Files         385      385            
  Lines      121302   121467   +165     
========================================
+ Hits        96526    96561    +35     
- Misses      24776    24906   +130

Flag	Coverage Δ
#kernel	`80% <ø> (ø)`	⬆️
#user	`67% <66%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a364048...c2e1080. Read the comment docs.

If the ZFS_COLOR env variable is set, then use ANSI color output in zpool status: - Column headers are bold - Degraded or offline pools/vdevs are yellow - Non-zero error counters and faulted vdevs/pools are red - The 'status:' and 'action:' sections are yellow if they're displaying a warning. This also includes a new 'faketty' function in libtest.shlib that is compatible with FreeBSD (code provided by @freqlabs). Signed-off-by: Tony Hutter <hutter2@llnl.gov>

tonyhutter · 2019-12-20T01:24:44Z

@behlendorf as a side note - we can probably remove ncurses-dev from buildbot since we ended up not using it.

behlendorf · 2019-12-20T01:25:42Z

Good call, will do.

tonyhutter added the Status: Work in Progress label Sep 21, 2019

tonyhutter force-pushed the ansi2 branch from d81c13d to be9e1b6 Compare October 5, 2019 00:16

tonyhutter force-pushed the ansi2 branch 6 times, most recently from 947f126 to 51e98a4 Compare October 21, 2019 20:26

tonyhutter mentioned this pull request Oct 22, 2019

Add in ncurses-devel openzfs/zfs-buildbot#172

Merged

tonyhutter force-pushed the ansi2 branch from 51e98a4 to 9c6af0f Compare October 22, 2019 19:43

ahrens added the Type: Feature Feature request or new feature label Nov 13, 2019

behlendorf reviewed Nov 20, 2019

View reviewed changes

tonyhutter force-pushed the ansi2 branch 5 times, most recently from e404bf3 to a9ebd98 Compare November 22, 2019 00:05

tonyhutter force-pushed the ansi2 branch 3 times, most recently from 93905e2 to ca77753 Compare November 26, 2019 16:52

tonyhutter removed the Status: Work in Progress label Nov 26, 2019

behlendorf reviewed Dec 6, 2019

View reviewed changes

tonyhutter force-pushed the ansi2 branch 2 times, most recently from 385ddd9 to e74fa97 Compare December 11, 2019 23:51

behlendorf approved these changes Dec 18, 2019

View reviewed changes

behlendorf requested review from a user and lundman December 18, 2019 02:05

lundman approved these changes Dec 18, 2019

View reviewed changes

ghost reviewed Dec 18, 2019

View reviewed changes

Comment thread cmd/zpool/zpool_main.c Outdated

Comment thread cmd/zpool/zpool_main.c Outdated

Comment thread include/libzutil.h Outdated

Comment thread lib/libzfs/libzfs_util.c Outdated

Comment thread tests/zfs-tests/tests/functional/cli_root/zpool/zpool_colors.ksh Outdated

tonyhutter force-pushed the ansi2 branch from e74fa97 to 0d3bcf0 Compare December 18, 2019 23:33

behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 18, 2019

ghost suggested changes Dec 19, 2019

View reviewed changes

Comment thread tests/zfs-tests/tests/functional/cli_root/zpool/zpool_colors.ksh Outdated

tonyhutter force-pushed the ansi2 branch from 0d3bcf0 to cf4ba15 Compare December 19, 2019 18:23

ghost suggested changes Dec 19, 2019

View reviewed changes

Comment thread tests/zfs-tests/tests/functional/cli_root/zpool/zpool_colors.ksh Outdated

tonyhutter force-pushed the ansi2 branch from cf4ba15 to c2e1080 Compare December 19, 2019 18:51

ghost approved these changes Dec 19, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 19, 2019

behlendorf merged commit 9fb2771 into openzfs:master Dec 20, 2019

Crow-Control mentioned this pull request Dec 24, 2019

Introduce ZSTD compression to ZFS #9735

Closed

9 of 12 tasks

ethan-coe-renner mentioned this pull request Dec 13, 2022

Colorize zfs diff output #14288

Merged

5 of 13 tasks

ethan-coe-renner mentioned this pull request Jan 4, 2023

Colorize zfs list output #14350

Closed

5 of 13 tasks

ethan-coe-renner mentioned this pull request Feb 3, 2023

Colorize zpool iostat output #14459

Closed

5 of 13 tasks

Conversation

tonyhutter commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tonyhutter commented Oct 22, 2019

Uh oh!

ahrens commented Nov 13, 2019

Uh oh!

ahrens commented Nov 13, 2019

Uh oh!

tonyhutter commented Nov 14, 2019

Uh oh!

lundman commented Nov 14, 2019

Uh oh!

spacelama commented Nov 14, 2019

Uh oh!

tonyhutter commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardelling commented Nov 15, 2019

Uh oh!

Uh oh!

behlendorf Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

behlendorf Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

behlendorf Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

behlendorf Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonyhutter commented Nov 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonyhutter commented Dec 12, 2019

Uh oh!

behlendorf commented Dec 18, 2019

Uh oh!

lundman commented Dec 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonyhutter commented Dec 18, 2019

Uh oh!

codecov Bot commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

tonyhutter commented Dec 20, 2019

Uh oh!

behlendorf commented Dec 20, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

tonyhutter commented Sep 21, 2019 •

edited

Loading

tonyhutter commented Nov 14, 2019 •

edited

Loading

codecov Bot commented Dec 19, 2019 •

edited

Loading