Date: Mon, 3 Apr 2017 06:52:02 +0000 (UTC) From: Bruce Evans <bde@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r316443 - head/usr.sbin/vidcontrol Message-ID: <201704030652.v336q21V028417@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bde Date: Mon Apr 3 06:52:02 2017 New Revision: 316443 URL: https://svnweb.freebsd.org/changeset/base/316443 Log: Fix some parsing and error handling bugs. r146736 added an undocumented syntax and many bugs handling it. The documented syntax is "... [mode] [fg [bg]] [show]", where it is critical for reducing ambiguity and keeping things simple that the mode is parsed first. r146736 added buggy support for "... [mode] [fg [bg]] [show] [mode] [fg [bg]]". One error was that after for failing to set a partially-supported graphics mode, argv[optind] remains pointing to the mode so doesn't match the first [fg [bg]], so the setting is attempted again, with slightly worse error handling. Fix this by removing it (support for the trailing '[mode] [fg [bg]]') and cleaning up. The cleanups are mostly to remove convolutions and bugs that didn't work to handle the ambiguous syntax '[fg [bg]] [fg [bg]]' when [mode] and [show] are not present. Globals were set to allow repeating the color settings at the end. The functions that set the colors earlier were misnamed from set* to get*. All that they "got" is is settings from argv. They applied the settings to the kernel and the globals. Fix restoration of colors in revert() by restoring 2 after the mode change. Colors should not need to be restored, but a bug in scteken clobbers them on any mode change, including ones for restoration. Don't move the restoration of the other 3. Teken doesn't clobber them on mode changes because it doesn't support them at all (sc still supports the border color, but only using a non-teken ioctl). Add restoration of colors after a successful mode change to work around the scteken bug there too. The bug was previously masked by the general setting of colors at the end. Fix a longstanding parsing/error handling bug by exiting almost immediately after matching the [mode] arg but failing to set the mode. Just revert if necessary. Don't return to continue parsing but do it wrong. This bug caused spamming the output with a usage() message and exiting with status 1 whenever [mode] is not present bug [fg [bg]] or [show]. The exit code 1 was actualy an ambiguous internal code for failure to match [mode] or failure to set [mode]. This 1 was obfuscated by spelling it EXIT_FAILURE, but actual exit codes spell EXIT_FAILURE as 1. Remove another global which could have been used to disambiguate this but was only used to micro-optimize the (unnecessary except for other bugs) setting of colors at the end. Modified: head/usr.sbin/vidcontrol/vidcontrol.c Modified: head/usr.sbin/vidcontrol/vidcontrol.c ============================================================================== --- head/usr.sbin/vidcontrol/vidcontrol.c Mon Apr 3 06:14:23 2017 (r316442) +++ head/usr.sbin/vidcontrol/vidcontrol.c Mon Apr 3 06:52:02 2017 (r316443) @@ -94,10 +94,6 @@ static int hex = 0; static int vesa_cols; static int vesa_rows; static int font_height; -static int colors_changed; -static int video_mode_changed; -static int normal_fore_color, normal_back_color; -static int revers_fore_color, revers_back_color; static int vt4_mode = 0; static struct vid_info info; static struct video_info new_mode_info; @@ -140,11 +136,6 @@ init(void) if (ioctl(0, CONS_MODEINFO, &cur_info.video_mode_info) == -1) errc(1, errno, "getting video mode parameters"); - - normal_fore_color = cur_info.console_info.mv_norm.fore; - normal_back_color = cur_info.console_info.mv_norm.back; - revers_fore_color = cur_info.console_info.mv_rev.fore; - revers_back_color = cur_info.console_info.mv_rev.back; } @@ -163,8 +154,6 @@ revert(void) ioctl(0, VT_ACTIVATE, cur_info.active_vty); fprintf(stderr, "\033[=%dA", cur_info.console_info.mv_ovscan); - fprintf(stderr, "\033[=%dF", cur_info.console_info.mv_norm.fore); - fprintf(stderr, "\033[=%dG", cur_info.console_info.mv_norm.back); fprintf(stderr, "\033[=%dH", cur_info.console_info.mv_rev.fore); fprintf(stderr, "\033[=%dI", cur_info.console_info.mv_rev.back); @@ -185,6 +174,10 @@ revert(void) ioctl(0, KDRASTER, size); } + + /* Restore some colors last since mode setting forgets some. */ + fprintf(stderr, "\033[=%dF", cur_info.console_info.mv_norm.fore); + fprintf(stderr, "\033[=%dG", cur_info.console_info.mv_norm.back); } @@ -657,7 +650,7 @@ set_cursor_type(char *appearance) * Set the video mode. */ -static int +static void video_mode(int argc, char **argv, int *mode_index) { static struct { @@ -728,10 +721,11 @@ video_mode(int argc, char **argv, int *m } if (modes[i].name == NULL) - return EXIT_FAILURE; + return; if (ioctl(0, mode, NULL) < 0) { - warn("cannot set videomode"); - return EXIT_FAILURE; + ioerr = errno; + revert(); + errc(1, ioerr, "cannot set videomode"); } } @@ -805,16 +799,19 @@ video_mode(int argc, char **argv, int *m else ioctl(0, _IO('S', cur_mode), NULL); revert(); - warnc(ioerr, "cannot activate raster display"); - return EXIT_FAILURE; + errc(1, ioerr, + "cannot activate raster display"); } } - video_mode_changed = 1; + /* Recover from mode setting forgetting colors. */ + fprintf(stderr, "\033[=%dF", + cur_info.console_info.mv_norm.fore); + fprintf(stderr, "\033[=%dG", + cur_info.console_info.mv_norm.back); (*mode_index)++; } - return EXIT_SUCCESS; } @@ -836,67 +833,47 @@ get_color_number(char *color) /* - * Get normal text and background colors. + * Set normal text and background colors. */ static void -get_normal_colors(int argc, char **argv, int *_index) +set_normal_colors(int argc, char **argv, int *_index) { int color; if (*_index < argc && (color = get_color_number(argv[*_index])) != -1) { (*_index)++; fprintf(stderr, "\033[=%dF", color); - normal_fore_color=color; - colors_changed = 1; if (*_index < argc && (color = get_color_number(argv[*_index])) != -1) { (*_index)++; fprintf(stderr, "\033[=%dG", color); - normal_back_color=color; } } } /* - * Get reverse text and background colors. + * Set reverse text and background colors. */ static void -get_reverse_colors(int argc, char **argv, int *_index) +set_reverse_colors(int argc, char **argv, int *_index) { int color; if ((color = get_color_number(argv[*(_index)-1])) != -1) { fprintf(stderr, "\033[=%dH", color); - revers_fore_color=color; - colors_changed = 1; if (*_index < argc && (color = get_color_number(argv[*_index])) != -1) { (*_index)++; fprintf(stderr, "\033[=%dI", color); - revers_back_color=color; } } } /* - * Set normal and reverse foreground and background colors. - */ - -static void -set_colors(void) -{ - fprintf(stderr, "\033[=%dF", normal_fore_color); - fprintf(stderr, "\033[=%dG", normal_back_color); - fprintf(stderr, "\033[=%dH", revers_fore_color); - fprintf(stderr, "\033[=%dI", revers_back_color); -} - - -/* * Switch to virtual terminal #arg. */ @@ -1342,7 +1319,6 @@ main(int argc, char **argv) char *font, *type, *termmode; const char *opts; int dumpmod, dumpopt, opt; - int reterr; vt4_mode = is_vt4(); @@ -1435,7 +1411,7 @@ main(int argc, char **argv) dumpmod = DUMP_FMT_TXT; break; case 'r': - get_reverse_colors(argc, argv, &optind); + set_reverse_colors(argc, argv, &optind); break; case 'S': set_lockswitch(optarg); @@ -1461,25 +1437,19 @@ main(int argc, char **argv) if (dumpmod != 0) dump_screen(dumpmod, dumpopt); - reterr = video_mode(argc, argv, &optind); - get_normal_colors(argc, argv, &optind); + video_mode(argc, argv, &optind); + set_normal_colors(argc, argv, &optind); if (optind < argc && !strcmp(argv[optind], "show")) { test_frame(); optind++; } - video_mode(argc, argv, &optind); if (termmode != NULL) set_terminal_mode(termmode); - get_normal_colors(argc, argv, &optind); - - if (colors_changed || video_mode_changed) - set_colors(); - if ((optind != argc) || (argc == 1)) usage(); - return reterr; + return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201704030652.v336q21V028417>