[PATCH 00/17] pull: almost complete setterm refactoring

[PATCH 00/17] pull: almost complete setterm refactoring

[Sami Kerola]

Hello,

Here is bunch of patches to setterm, with the intention to make the code
to look a little bit more fresh.  I admit the second patch being
practically impossible to review because there is so much going on in it. 
Unfortunately I could not figure out how to change how options work, and
keep the change small.

Lets hope this does not break anything, while the change should make the
command more robust in various ways.


The following changes since commit 1efded63dfe85d62bcd8f0ff29ea6b62aa5cac74:

  Merge branch 'tests-mount' of https://github.com/rudimeier/util-linux (2014-05-16 14:30:05 +0200)

are available in the git repository at:

  git://github.com/kerolasa/lelux-utiliteetit.git 2014wk19

for you to fetch changes up to 19d616172ad30e375056ab3ce86d0166a840773a:

  setterm: add set_blanking() action (2014-05-18 14:55:23 +0100)

Sami Kerola (17):
  setterm: clean up includes
  setterm: use getopt_long_only() for option parsing
  setterm: use string utils to numeric parsing
  setterm: move show_tabs() and screendump() functions
  setterm: remove usage comment segment
  setterm: add option control structure
  setterm: add init_terminal() to make main() shorter
  setterm: clean up screendump()
  setterm: remove devfs and /dev/vcsa0 support
  setterm: make -msglevel 0 to work as is did earlier
  setterm: correct usage() bright color argument
  setterm: improve perform_sequence() coding style
  setterm: tell user when options does not effect
  setterm: improve error messages
  setterm: various visual terminal effects are not console specific
  setterm: mark some options to be exclusive with each other
  setterm: add set_blanking() action

 bash-completion/setterm  |   19 +-
 term-utils/Makemodule.am |    2 +-
 term-utils/setterm.c     | 1621 +++++++++++++++++++++-------------------------
 3 files changed, 761 insertions(+), 881 deletions(-)

-- 
1.9.2

[PATCH 01/17] setterm: clean up includes

[Sami Kerola]

Use klogctl(2) from sys/klog.h just like dmesg(1).  The rest is just
reordering, and indenting.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 7a3131f..e6385ac 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -92,48 +92,38 @@
  *   -store stores the terminal's current rendering options as the default
  *      values.  */
 
+#include <ctype.h>
+#include <errno.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <errno.h>
-#include <ctype.h>
-#include <unistd.h>
-#include <termios.h>
 #include <string.h>
-#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/klog.h>
+#include <sys/param.h>		/* for MAXPATHLEN */
+#include <sys/time.h>
+#include <termios.h>
+#include <unistd.h>
 
 #ifndef NCURSES_CONST
-#define NCURSES_CONST const	/* define before including term.h */
+# define NCURSES_CONST const	/* define before including term.h */
 #endif
 #ifdef HAVE_NCURSES_H
-#include <ncurses.h>
+# include <ncurses.h>
 #elif defined(HAVE_NCURSES_NCURSES_H)
-#include <ncurses/ncurses.h>
+# include <ncurses/ncurses.h>
 #endif
 /* must include after ncurses.h */
 #include <term.h>
 
-#include <sys/param.h>		/* for MAXPATHLEN */
-#include <sys/ioctl.h>
-#include <sys/time.h>
 #ifdef HAVE_LINUX_TIOCL_H
-#include <linux/tiocl.h>
+# include <linux/tiocl.h>
 #endif
 
 #include "c.h"
-#include "xalloc.h"
-#include "nls.h"
 #include "closestream.h"
-
-#if defined(__GNU_LIBRARY__) && __GNU_LIBRARY__ < 5
-#ifndef __alpha__
-# include <linux/unistd.h>
-#define __NR_klogctl __NR_syslog
-_syscall3(int, klogctl, int, type, char*, buf, int, len);
-#else /* __alpha__ */
-#define klogctl syslog
-#endif
-#endif
-extern int klogctl(int type, char *buf, int len);
+#include "nls.h"
+#include "xalloc.h"
 
 /* Constants. */
 
-- 
1.9.2

[PATCH 02/17] setterm: use getopt_long_only() for option parsing

[Sami Kerola]

The getopt_long_only() has advantage of allowing one and two hyphen
versions of options, for example -help and --help.  Secondly the getopt
function family from libc can ensure some options requiring arguments,
while other are optional, and some should not accept argument at all.
That makes option parsing more robust.

Unfortunately retiring the old option parsing makes this change greater
than preferred.  Assuming not mistakes happen the new code works
functionally exactly as the old did.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 916 ++++++++++++++++++++++++---------------------------
 1 file changed, 426 insertions(+), 490 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index e6385ac..25baff7 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -95,6 +95,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -217,7 +218,7 @@ int opt_tb_array[TABS_MAX + 1];	/* Array for tab list */
 int opt_msglevel_num;
 int opt_ps_mode, opt_pd_min;	/* powersave mode/powerdown time */
 
-char opt_sn_name[PATH_MAX + 1] = "screen.dump";
+char opt_sn_name[PATH_MAX] = "screen.dump";
 
 static void screendump(int vcnum, FILE *F);
 
@@ -226,416 +227,231 @@ static void screendump(int vcnum, FILE *F);
  * Note that it is an error for a given option to be invoked more than once.
  */
 
-static void
-parse_term(int argc, char **argv, int *option, char **ttyname, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Term flag to set. */
-	/* ttyname: Terminal name to set. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -term specification. */
-
-	if (argc != 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1)
-		*ttyname = argv[0];
-}
-
-static void
-parse_none(int argc, char **argv __attribute__ ((__unused__)), int *option, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Term flag to set. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a parameterless specification. */
-
-	if (argc != 0 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-}
-
-static void
-parse_switch(int argc, char **argv, int *option, int *opt_on, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Option flag to set. */
-	/* opt_on: Boolean option switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a boolean (on/off) specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		if (strcmp(argv[0], "on") == 0)
-			*opt_on = TRUE;
-		else if (strcmp(argv[0], "off") == 0)
-			*opt_on = FALSE;
-		else
-			*bad_arg = TRUE;
-	} else {
-		*opt_on = TRUE;
-	}
+static int parse_switch(const char *arg, const char *t, const char *f)
+{
+	if (strcmp(arg, t) == 0)
+		return 1;
+	else if (strcmp(arg, f) == 0)
+		return 0;
+	errx(EXIT_FAILURE, _("argument error: %s"), arg);
 }
 
-static void
-par_color(int argc, char **argv, int *option, int *opt_color, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Color flag to set. */
-	/* opt_color: Color to set. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -foreground or -background specification. */
-
-	if (argc != 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		if (strcmp(argv[0], "black") == 0)
-			*opt_color = BLACK;
-		else if (strcmp(argv[0], "red") == 0)
-			*opt_color = RED;
-		else if (strcmp(argv[0], "green") == 0)
-			*opt_color = GREEN;
-		else if (strcmp(argv[0], "yellow") == 0)
-			*opt_color = YELLOW;
-		else if (strcmp(argv[0], "blue") == 0)
-			*opt_color = BLUE;
-		else if (strcmp(argv[0], "magenta") == 0)
-			*opt_color = MAGENTA;
-		else if (strcmp(argv[0], "cyan") == 0)
-			*opt_color = CYAN;
-		else if (strcmp(argv[0], "white") == 0)
-			*opt_color = WHITE;
-		else if (strcmp(argv[0], "default") == 0)
-			*opt_color = DEFAULT;
-		else if (isdigit(argv[0][0]))
-			*opt_color = atoi(argv[0]);
-		else
-			*bad_arg = TRUE;
-
-		if(*opt_color < BLACK || DEFAULT < *opt_color || *opt_color == GREY)
-			*bad_arg = TRUE;
+static int parse_febg_color(const char *arg)
+{
+	if (strcmp(arg, "black") == 0)
+		return BLACK;
+	else if (strcmp(arg, "red") == 0)
+		return RED;
+	else if (strcmp(arg, "green") == 0)
+		return GREEN;
+	else if (strcmp(arg, "yellow") == 0)
+		return YELLOW;
+	else if (strcmp(arg, "blue") == 0)
+		return BLUE;
+	else if (strcmp(arg, "magenta") == 0)
+		return MAGENTA;
+	else if (strcmp(arg, "cyan") == 0)
+		return CYAN;
+	else if (strcmp(arg, "white") == 0)
+		return WHITE;
+	else if (strcmp(arg, "default") == 0)
+		return DEFAULT;
+	else if (isdigit(arg[0])) {
+		int color;
+
+		color = atoi(arg);
+		if (color < BLACK || DEFAULT < color || color == GREY)
+			errx(EXIT_FAILURE, _("argument error: %s"), arg);
+		return color;
 	}
+	errx(EXIT_FAILURE, _("argument error: %s"), arg);
 }
 
-static void
-par_color2(int argc, char **argv, int *option, int *opt_color, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Color flag to set. */
-	/* opt_color: Color to set. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -ulcolor or -hbcolor specification. */
-
-	if (!argc || argc > 2 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	*opt_color = 0;
-	if (argc == 2) {
-		if (strcmp(argv[0], "bright") == 0)
-			*opt_color = GREY;
-		else {
-			*bad_arg = TRUE;
-			return;
-		}
-	}
-	if (argc) {
-		if (strcmp(argv[argc-1], "black") == 0) {
-			if(*opt_color)
-				*bad_arg = TRUE;
-			else
-				*opt_color = BLACK;
-		} else if (strcmp(argv[argc-1], "grey") == 0) {
-			if(*opt_color)
-				*bad_arg = TRUE;
-			else
-				*opt_color = GREY;
-		} else if (strcmp(argv[argc-1], "red") == 0)
-			*opt_color |= RED;
-		else if (strcmp(argv[argc-1], "green") == 0)
-			*opt_color |= GREEN;
-		else if (strcmp(argv[argc-1], "yellow") == 0)
-			*opt_color |= YELLOW;
-		else if (strcmp(argv[argc-1], "blue") == 0)
-			*opt_color |= BLUE;
-		else if (strcmp(argv[argc-1], "magenta") == 0)
-			*opt_color |= MAGENTA;
-		else if (strcmp(argv[argc-1], "cyan") == 0)
-			*opt_color |= CYAN;
-		else if (strcmp(argv[argc-1], "white") == 0)
-			*opt_color |= WHITE;
-		else if (isdigit(argv[argc-1][0]))
-			*opt_color = atoi(argv[argc-1]);
-		else
-			*bad_arg = TRUE;
-		if(*opt_color < BLACK || DEFAULT < *opt_color )
-			*bad_arg = TRUE;
-	}
+static int parse_ulhb_color(char **argv, int *optind)
+{
+	char *color_name;
+	int bright = 0;
+	int color = -1;
+
+	if (argv[*optind] && strcmp(argv[*optind - 1], "bright") == 0) {
+		bright = 1;
+		color_name = argv[*optind];
+		(*optind)++;
+	} else
+		color_name = argv[*optind - 1];
+
+	if (strcmp(color_name, "black") == 0)
+		color = BLACK;
+	else if (strcmp(color_name, "grey") == 0)
+		color = GREY;
+	else if (strcmp(color_name, "red") == 0)
+		color = RED;
+	else if (strcmp(color_name, "green") == 0)
+		color = GREEN;
+	else if (strcmp(color_name, "yellow") == 0)
+		color = YELLOW;
+	else if (strcmp(color_name, "blue") == 0)
+		color = BLUE;
+	else if (strcmp(color_name, "magenta") == 0)
+		color = MAGENTA;
+	else if (strcmp(color_name, "cyan") == 0)
+		color = CYAN;
+	else if (strcmp(color_name, "white") == 0)
+		color = WHITE;
+	else if (isdigit(color_name[0]))
+		color = atoi(color_name);
+
+	if (color < BLACK || DEFAULT < color)
+		errx(EXIT_FAILURE, _("argument error: %s"), color_name);
+	if (bright && (color == BLACK || color == GREY))
+		errx(EXIT_FAILURE, _("argument error: bright %s is not supported"), color_name);
+
+	return color;
 }
 
-static void
-parse_clear(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all: Clear all switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -clear specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		if (strcmp(argv[0], "all") == 0)
-			*opt_all = TRUE;
-		else if (strcmp(argv[0], "rest") == 0)
-			*opt_all = FALSE;
-		else
-			*bad_arg = TRUE;
-	} else {
-		*opt_all = TRUE;
+static char *find_optional_arg(char **argv, char *optarg, int *optind)
+{
+	char *arg;
+	if (optarg)
+		return optarg;
+	else {
+		arg = argv[*optind];
+		if (!arg || arg[0] == '-')
+			return NULL;
 	}
+	(*optind)++;
+	return arg;
 }
 
-static void
-parse_blank(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all: Clear all switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -blank specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		if (!strcmp(argv[0], "force"))
-			*opt_all = BLANKSCREEN;
-		else if (!strcmp(argv[0], "poke"))
-			*opt_all = UNBLANKSCREEN;
-		else {
-			*opt_all = atoi(argv[0]);
-			if (*opt_all < 0 || BLANK_MAX < *opt_all)
-				*bad_arg = TRUE;
-		}
-	} else {
-		*opt_all = BLANKEDSCREEN;
+static int parse_blank(char **argv, char *optarg, int *optind)
+{
+	char *arg;
+
+	arg = find_optional_arg(argv, optarg, optind);
+	if (!arg)
+		return BLANKEDSCREEN;
+	if (!strcmp(arg, "force"))
+		return BLANKSCREEN;
+	else if (!strcmp(arg, "poke"))
+		return UNBLANKSCREEN;
+	else {
+		int ret = -1;
+
+		if (isdigit(arg[0]))
+			ret = atoi(arg);
+		if (ret < 0 || BLANK_MAX < ret)
+			errx(EXIT_FAILURE, _("argument error: %s"), arg);
+		return ret;
 	}
+	/* should be impossible to reach */
+	abort();
 }
 
-static void
-parse_powersave(int argc, char **argv, int *option, int *opt_mode, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: powersave flag to set. */
-	/* opt_mode: Powersaving mode, defined in vesa_blank.c */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -powersave mode specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		if (strcmp(argv[0], "on") == 0)
-			*opt_mode = VESA_BLANK_MODE_SUSPENDV;
-		else if (strcmp(argv[0], "vsync") == 0)
-			*opt_mode = VESA_BLANK_MODE_SUSPENDV;
-		else if (strcmp(argv[0], "hsync") == 0)
-			*opt_mode = VESA_BLANK_MODE_SUSPENDH;
-		else if (strcmp(argv[0], "powerdown") == 0)
-			*opt_mode = VESA_BLANK_MODE_POWERDOWN;
-		else if (strcmp(argv[0], "off") == 0)
-			*opt_mode = VESA_BLANK_MODE_OFF;
-		else
-			*bad_arg = TRUE;
-	} else {
-		*opt_mode = VESA_BLANK_MODE_OFF;
-	}
+static int parse_powersave(const char *arg)
+{
+	if (strcmp(arg, "on") == 0)
+		return VESA_BLANK_MODE_SUSPENDV;
+	else if (strcmp(arg, "vsync") == 0)
+		return VESA_BLANK_MODE_SUSPENDV;
+	else if (strcmp(arg, "hsync") == 0)
+		return VESA_BLANK_MODE_SUSPENDH;
+	else if (strcmp(arg, "powerdown") == 0)
+		return VESA_BLANK_MODE_POWERDOWN;
+	else if (strcmp(arg, "off") == 0)
+		return VESA_BLANK_MODE_OFF;
+	errx(EXIT_FAILURE, _("argument error: %s"), arg);
 }
 
-static void
-parse_msglevel(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all: Clear all switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		*opt_all = atoi(argv[0]);
-		if (*opt_all < CONSOLE_LEVEL_MIN || CONSOLE_LEVEL_MAX <  *opt_all)
-			*bad_arg = TRUE;
-	} else {
-		*opt_all = -1;
-	}
-}
+static int parse_msglevel(const char *arg)
+{
+	int ret = CONSOLE_LEVEL_MIN - 1;
 
-static void
-parse_snap(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all: Clear all switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -dump or -append specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		*opt_all = atoi(argv[0]);
-		if ((*opt_all <= 0))
-			*bad_arg = TRUE;
-	} else {
-		*opt_all = 0;
-	}
+	if (isdigit(arg[0]))
+		ret = atoi(arg);
+	if (ret < CONSOLE_LEVEL_MIN || CONSOLE_LEVEL_MAX < ret)
+		errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	return ret;
 }
 
-static void
-parse_snapfile(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all: Clear all switch to set or reset. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a -file specification. */
-
-	if (argc != 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	memset(opt_all, 0, PATH_MAX + 1);
-	if (argc == 1)
-		strncpy((char *)opt_all, argv[0], PATH_MAX);
+static int parse_snap(char **argv, char *optarg, int *optind)
+{
+	int ret = 0;
+	char *arg;
+
+	arg = find_optional_arg(argv, optarg, optind);
+	if (!arg)
+		return 0;
+	if (isdigit(arg[0]))
+		ret = atoi(arg);
+	if (ret < 1)
+		errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	return ret;
 }
 
-static void
-parse_tabs(int argc, char **argv, int *option, int *tab_array, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* tab_array: Array of tabs */
-	/* bad_arg: Set to true if an error is detected. */
-
-	if (*option || TABS_MAX < argc)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	tab_array[argc] = -1;
-	while(argc--) {
-		tab_array[argc] = atoi(argv[argc]);
-		if (tab_array[argc] < 1 || TABS_MAX < tab_array[argc]) {
-			*bad_arg = TRUE;
-			return;
-		}
-	}
-}
+static void parse_tabs(char **argv, char *optarg, int *optind, int *tab_array)
+{
+	int i = 0;
 
-static void
-parse_clrtabs(int argc, char **argv, int *option, int *tab_array, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* tab_array: Array of tabs */
-	/* bad_arg: Set to true if an error is detected. */
-
-	if (*option || TABS_MAX < argc)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if(argc == 0) {
-		tab_array[0] = -1;
-		return;
+	if (optarg) {
+		tab_array[i] = atoi(optarg);
+		i++;
 	}
-	tab_array[argc] = -1;
-	while(argc--) {
-		tab_array[argc] = atoi(argv[argc]);
-		if(tab_array[argc] < 1 || TABS_MAX < tab_array[argc]) {
-			*bad_arg = TRUE;
-			return;
-		}
+	while (argv[*optind]) {
+		if (TABS_MAX < i)
+			errx(EXIT_FAILURE, _("too many tabs"));
+		if (argv[*optind][0] == '-')
+			break;
+		if (isdigit(argv[*optind][0]))
+			tab_array[i] = atoi(argv[*optind]);
+		else
+			break;
+		(*optind)++;
+		i++;
 	}
+	tab_array[i] = -1;
 }
 
-static void
-parse_regtabs(int argc, char **argv, int *option, int *opt_len, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_len: Regular tab length. */
-	/* bad_arg: Set to true if an error is detected. */
-
-	if (*option || argc > 1)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if(argc == 0) {
-		*opt_len = DEFAULT_TAB_LEN;
-		return;
-	}
-	*opt_len = atoi(argv[0]);
-	if(*opt_len < 1 || TABS_MAX < *opt_len) {
-		*bad_arg = TRUE;
-		return;
-	}
+static int parse_regtabs(char **argv, char *optarg, int *optind)
+{
+	int ret;
+	char *arg;
+
+	arg = find_optional_arg(argv, optarg, optind);
+	if (!arg)
+		return DEFAULT_TAB_LEN;
+	ret = atoi(arg);
+	if (ret < 1 || TABS_MAX < ret)
+		errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	return ret;
 }
 
-
-static void
-parse_blength(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse  -blength specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		*opt_all = atoi(argv[0]);
-		if (BLENGTH_MAX < *opt_all)
-			*bad_arg = TRUE;
-	} else {
-		*opt_all = 0;
-	}
+static int parse_blength(char **argv, char *optarg, int *optind)
+{
+	int ret = -1;
+	char *arg;
+
+	arg = find_optional_arg(argv, optarg, optind);
+	if (!arg)
+		return 0;
+	if (isdigit(arg[0]))
+		ret = atoi(arg);
+	if (ret < 0 || BLENGTH_MAX < ret)
+		errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	return ret;
 }
 
-static void
-parse_bfreq(int argc, char **argv, int *option, int *opt_all, int *bad_arg) {
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* option: Clear flag to set. */
-	/* opt_all */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse  -bfreq specification. */
-
-	if (argc > 1 || *option)
-		*bad_arg = TRUE;
-	*option = TRUE;
-	if (argc == 1) {
-		*opt_all = atoi(argv[0]);
-	} else {
-		*opt_all = 0;
-	}
+static int parse_bfreq(char **argv, char *optarg, int *optind)
+{
+	char *arg;
+
+	arg = find_optional_arg(argv, optarg, optind);
+	if (!arg)
+		return 0;
+	if (isdigit(arg[0]))
+		return atoi(arg);
+	return 0;
 }
 
-
 static void
 show_tabs(void) {
 	int i, co = tigetnum("cols");
@@ -696,7 +512,7 @@ usage(FILE *out) {
 	fputs(_(" -append <1-NR_CONSOLES>\n"), out);
 	fputs(_(" -file dumpfilename\n"), out);
 	fputs(_(" -msg <on|off>\n"), out);
-	fputs(_(" -msglevel <0-8>\n"), out);
+	fputs(_(" -msglevel <0-8>\n"), out);	/* FIXME: klogctl console_log range is 1-8 */
 	fputs(_(" -powersave <on|vsync|hsync|powerdown|off>\n"), out);
 	fputs(_(" -powerdown <0-60>\n"), out);
 	fputs(_(" -blength <0-2000>\n"), out);
@@ -709,91 +525,234 @@ usage(FILE *out) {
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-#define STRCMP(str1,str2) strncmp(str1,str2,strlen(str1))
+static void set_opt_flag(int *opt)
+{
+	if (*opt)
+		errx(EXIT_FAILURE, "duplicate use of an option");
+	*opt = 1;
+}
 
-static void
-parse_option(char *option, int argc, char **argv, int *bad_arg) {
-	/* option: Option with leading '-' removed. */
-	/* argc: Number of arguments for this option. */
-	/* argv: Arguments for this option. */
-	/* bad_arg: Set to true if an error is detected. */
-
-/* Parse a single specification. */
-
-	if (STRCMP(option, "term") == 0)
-		parse_term(argc, argv, &opt_term, &opt_te_terminal_name, bad_arg);
-	else if (STRCMP(option, "reset") == 0)
-		parse_none(argc, argv, &opt_reset, bad_arg);
-	else if (STRCMP(option, "initialize") == 0)
-		parse_none(argc, argv, &opt_initialize, bad_arg);
-	else if (STRCMP(option, "cursor") == 0)
-		parse_switch(argc, argv, &opt_cursor, &opt_cu_on, bad_arg);
-	else if (STRCMP(option, "repeat") == 0)
-		parse_switch(argc, argv, &opt_repeat, &opt_rep_on, bad_arg);
-	else if (STRCMP(option, "appcursorkeys") == 0)
-		parse_switch(argc, argv, &opt_appcursorkeys, &opt_appck_on, bad_arg);
-	else if (STRCMP(option, "linewrap") == 0)
-		parse_switch(argc, argv, &opt_linewrap, &opt_li_on, bad_arg);
-	else if (STRCMP(option, "default") == 0)
-		parse_none(argc, argv, &opt_default, bad_arg);
-	else if (STRCMP(option, "foreground") == 0)
-		par_color(argc, argv, &opt_foreground, &opt_fo_color, bad_arg);
-	else if (STRCMP(option, "background") == 0)
-		par_color(argc, argv, &opt_background, &opt_ba_color, bad_arg);
-	else if (STRCMP(option, "ulcolor") == 0)
-		par_color2(argc, argv, &opt_ulcolor, &opt_ul_color, bad_arg);
-	else if (STRCMP(option, "hbcolor") == 0)
-		par_color2(argc, argv, &opt_hbcolor, &opt_hb_color, bad_arg);
-	else if (STRCMP(option, "inversescreen") == 0)
-		parse_switch(argc, argv, &opt_inversescreen, &opt_invsc_on, bad_arg);
-	else if (STRCMP(option, "bold") == 0)
-		parse_switch(argc, argv, &opt_bold, &opt_bo_on, bad_arg);
-	else if (STRCMP(option, "half-bright") == 0)
-		parse_switch(argc, argv, &opt_halfbright, &opt_hb_on, bad_arg);
-	else if (STRCMP(option, "blink") == 0)
-		parse_switch(argc, argv, &opt_blink, &opt_bl_on, bad_arg);
-	else if (STRCMP(option, "reverse") == 0)
-		parse_switch(argc, argv, &opt_reverse, &opt_re_on, bad_arg);
-	else if (STRCMP(option, "underline") == 0)
-		parse_switch(argc, argv, &opt_underline, &opt_un_on, bad_arg);
-	else if (STRCMP(option, "store") == 0)
-		parse_none(argc, argv, &opt_store, bad_arg);
-	else if (STRCMP(option, "clear") == 0)
-		parse_clear(argc, argv, &opt_clear, &opt_cl_all, bad_arg);
-	else if (STRCMP(option, "tabs") == 0)
-		parse_tabs(argc, argv, &opt_tabs, opt_tb_array, bad_arg);
-	else if (STRCMP(option, "clrtabs") == 0)
-		parse_clrtabs(argc, argv, &opt_clrtabs, opt_tb_array, bad_arg);
-	else if (STRCMP(option, "regtabs") == 0)
-		parse_regtabs(argc, argv, &opt_regtabs, &opt_rt_len, bad_arg);
-	else if (STRCMP(option, "blank") == 0)
-		parse_blank(argc, argv, &opt_blank, &opt_bl_min, bad_arg);
-	else if (STRCMP(option, "dump") == 0)
-		parse_snap(argc, argv, &opt_snap, &opt_sn_num, bad_arg);
-	else if (STRCMP(option, "append") == 0)
-		parse_snap(argc, argv, &opt_append, &opt_sn_num, bad_arg);
-	else if (STRCMP(option, "file") == 0)
-		parse_snapfile(argc, argv, &opt_snapfile, (int *)opt_sn_name, bad_arg);
-	else if (STRCMP(option, "msg") == 0)
-		parse_switch(argc, argv, &opt_msg, &opt_msg_on, bad_arg);
-	else if (STRCMP(option, "msglevel") == 0)
-		parse_msglevel(argc, argv, &opt_msglevel, &opt_msglevel_num, bad_arg);
-	else if (STRCMP(option, "powersave") == 0)
-		parse_powersave(argc, argv, &opt_powersave, &opt_ps_mode, bad_arg);
-	else if (STRCMP(option, "powerdown") == 0)
-		parse_blank(argc, argv, &opt_powerdown, &opt_pd_min, bad_arg);
-	else if (STRCMP(option, "blength") == 0)
-		parse_blength(argc, argv, &opt_blength, &opt_blength_l, bad_arg);
-	else if (STRCMP(option, "bfreq") == 0)
-		parse_bfreq(argc, argv, &opt_bfreq, &opt_bfreq_f, bad_arg);
-	else if (STRCMP(option, "version") == 0) {
-		printf(_("%s from %s\n"), program_invocation_short_name,
-					  PACKAGE_STRING);
-		exit(EXIT_SUCCESS);
-	} else if (STRCMP(option, "help") == 0)
-		usage(stdout);
-	else
-		*bad_arg = TRUE;
+static void parse_option(int argc, char **argv)
+{
+	int c;
+	enum {
+		OPT_TERM = CHAR_MAX + 1,
+		OPT_RESET,
+		OPT_INITIALIZE,
+		OPT_CURSOR,
+		OPT_REPEAT,
+		OPT_APPCURSORKEYS,
+		OPT_LINEWRAP,
+		OPT_DEFAULT,
+		OPT_FOREGROUND,
+		OPT_BACKGROUND,
+		OPT_ULCOLOR,
+		OPT_HBCOLOR,
+		OPT_INVERSESCREEN,
+		OPT_BOLD,
+		OPT_HALF_BRIGHT,
+		OPT_BLINK,
+		OPT_REVERSE,
+		OPT_UNDERLINE,
+		OPT_STORE,
+		OPT_CLEAR,
+		OPT_TABS,
+		OPT_CLRTABS,
+		OPT_REGTABS,
+		OPT_BLANK,
+		OPT_DUMP,
+		OPT_APPEND,
+		OPT_FILE,
+		OPT_MSG,
+		OPT_MSGLEVEL,
+		OPT_POWERSAVE,
+		OPT_POWERDOWN,
+		OPT_BLENGTH,
+		OPT_BFREQ,
+		OPT_VERSION,
+		OPT_HELP
+	};
+	static const struct option longopts[] = {
+		{"term", required_argument, NULL, OPT_TERM},
+		{"reset", no_argument, NULL, OPT_RESET},
+		{"initialize", no_argument, NULL, OPT_INITIALIZE},
+		{"cursor", required_argument, NULL, OPT_CURSOR},
+		{"repeat", required_argument, NULL, OPT_REPEAT},
+		{"appcursorkeys", required_argument, NULL, OPT_APPCURSORKEYS},
+		{"linewrap", required_argument, NULL, OPT_LINEWRAP},
+		{"default", no_argument, NULL, OPT_DEFAULT},
+		{"foreground", required_argument, NULL, OPT_FOREGROUND},
+		{"background", required_argument, NULL, OPT_BACKGROUND},
+		{"ulcolor", required_argument, NULL, OPT_ULCOLOR},
+		{"ulcolor", required_argument, NULL, OPT_ULCOLOR},
+		{"hbcolor", required_argument, NULL, OPT_HBCOLOR},
+		{"hbcolor", required_argument, NULL, OPT_HBCOLOR},
+		{"inversescreen", required_argument, NULL, OPT_INVERSESCREEN},
+		{"bold", required_argument, NULL, OPT_BOLD},
+		{"half-bright", required_argument, NULL, OPT_HALF_BRIGHT},
+		{"blink", required_argument, NULL, OPT_BLINK},
+		{"reverse", required_argument, NULL, OPT_REVERSE},
+		{"underline", required_argument, NULL, OPT_UNDERLINE},
+		{"store", no_argument, NULL, OPT_STORE},
+		{"clear", required_argument, NULL, OPT_CLEAR},
+		{"tabs", optional_argument, NULL, OPT_TABS},
+		{"clrtabs", optional_argument, NULL, OPT_CLRTABS},
+		{"regtabs", optional_argument, NULL, OPT_REGTABS},
+		{"blank", optional_argument, NULL, OPT_BLANK},
+		{"dump", optional_argument, NULL, OPT_DUMP},
+		{"append", required_argument, NULL, OPT_APPEND},
+		{"file", required_argument, NULL, OPT_FILE},
+		{"msg", required_argument, NULL, OPT_MSG},
+		{"msglevel", required_argument, NULL, OPT_MSGLEVEL},
+		{"powersave", required_argument, NULL, OPT_POWERSAVE},
+		{"powerdown", optional_argument, NULL, OPT_POWERDOWN},
+		{"blength", optional_argument, NULL, OPT_BLENGTH},
+		{"bfreq", optional_argument, NULL, OPT_BFREQ},
+		{"version", no_argument, NULL, OPT_VERSION},
+		{"help", no_argument, NULL, OPT_HELP},
+		{NULL, 0, NULL, 0}
+	};
+
+	while ((c = getopt_long_only(argc, argv, "", longopts, NULL)) != -1) {
+		switch (c) {
+		case OPT_TERM:
+			set_opt_flag(&opt_term);
+			opt_te_terminal_name = optarg;
+			break;
+		case OPT_RESET:
+			set_opt_flag(&opt_reset);
+			break;
+		case OPT_INITIALIZE:
+			set_opt_flag(&opt_initialize);
+			break;
+		case OPT_CURSOR:
+			set_opt_flag(&opt_cursor);
+			opt_cu_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_REPEAT:
+			set_opt_flag(&opt_repeat);
+			opt_rep_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_APPCURSORKEYS:
+			set_opt_flag(&opt_appcursorkeys);
+			opt_appck_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_LINEWRAP:
+			set_opt_flag(&opt_linewrap);
+			opt_li_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_DEFAULT:
+			set_opt_flag(&opt_default);
+			break;
+		case OPT_FOREGROUND:
+			set_opt_flag(&opt_foreground);
+			opt_fo_color = parse_febg_color(optarg);
+			break;
+		case OPT_BACKGROUND:
+			set_opt_flag(&opt_background);
+			opt_ba_color = parse_febg_color(optarg);
+			break;
+		case OPT_ULCOLOR:
+			set_opt_flag(&opt_ulcolor);
+			opt_ul_color = parse_ulhb_color(argv, &optind);
+			break;
+		case OPT_HBCOLOR:
+			set_opt_flag(&opt_hbcolor);
+			opt_hb_color = parse_ulhb_color(argv, &optind);
+			break;
+		case OPT_INVERSESCREEN:
+			set_opt_flag(&opt_inversescreen);
+			opt_invsc_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_BOLD:
+			set_opt_flag(&opt_bold);
+			opt_bo_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_HALF_BRIGHT:
+			set_opt_flag(&opt_halfbright);
+			opt_hb_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_BLINK:
+			set_opt_flag(&opt_blink);
+			opt_bl_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_REVERSE:
+			set_opt_flag(&opt_reverse);
+			opt_re_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_UNDERLINE:
+			set_opt_flag(&opt_underline);
+			opt_un_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_STORE:
+			set_opt_flag(&opt_store);
+			break;
+		case OPT_CLEAR:
+			set_opt_flag(&opt_clear);
+			opt_cl_all = parse_switch(optarg, "all", "reset");
+			break;
+		case OPT_TABS:
+			set_opt_flag(&opt_tabs);
+			parse_tabs(argv, optarg, &optind, opt_tb_array);
+			break;
+		case OPT_CLRTABS:
+			set_opt_flag(&opt_clrtabs);
+			parse_tabs(argv, optarg, &optind, opt_tb_array);
+			break;
+		case OPT_REGTABS:
+			set_opt_flag(&opt_regtabs);
+			opt_rt_len = parse_regtabs(argv, optarg, &optind);
+			break;
+		case OPT_BLANK:
+			set_opt_flag(&opt_blank);
+			opt_bl_min = parse_blank(argv, optarg, &optind);
+			break;
+		case OPT_DUMP:
+			set_opt_flag(&opt_snap);
+			opt_sn_num = parse_snap(argv, optarg, &optind);
+			break;
+		case OPT_APPEND:
+			set_opt_flag(&opt_append);
+			opt_sn_num = parse_snap(argv, optarg, &optind);
+			break;
+		case OPT_FILE:
+			set_opt_flag(&opt_snapfile);
+			strncpy(opt_sn_name, optarg, PATH_MAX);	/* FIXME: should use xstrncpy() */
+			opt_sn_name[PATH_MAX - 1] = 0;
+			break;
+		case OPT_MSG:
+			set_opt_flag(&opt_msg);
+			opt_msg_on = parse_switch(optarg, "on", "off");
+			break;
+		case OPT_MSGLEVEL:
+			set_opt_flag(&opt_msglevel);
+			opt_msglevel_num = parse_msglevel(optarg);
+			break;
+		case OPT_POWERSAVE:
+			set_opt_flag(&opt_powersave);
+			opt_ps_mode = parse_powersave(optarg);
+			break;
+		case OPT_POWERDOWN:
+			set_opt_flag(&opt_powerdown);
+			opt_pd_min = parse_blank(argv, optarg, &optind);
+			break;
+		case OPT_BLENGTH:
+			set_opt_flag(&opt_blength);
+			opt_blength_l = parse_blength(argv, optarg, &optind);
+			break;
+		case OPT_BFREQ:
+			set_opt_flag(&opt_bfreq);
+			opt_bfreq_f = parse_bfreq(argv, optarg, &optind);
+			break;
+		case OPT_VERSION:
+			printf(UTIL_LINUX_VERSION);
+			exit(EXIT_SUCCESS);
+		case OPT_HELP:
+			usage(stdout);
+		default:
+			usage(stderr);
+		}
+	}
 }
 
 /* End of command line parsing routines. */
@@ -1153,8 +1112,6 @@ error:
 
 int
 main(int argc, char **argv) {
-	int bad_arg = FALSE;		/* Set if error in arguments. */
-	int arg, modifier;
 	char *term;			/* Terminal type. */
 	int vcterm;			/* Set if terminal is a virtual console. */
 	int errret;
@@ -1165,31 +1122,10 @@ main(int argc, char **argv) {
 	atexit(close_stdout);
 
 	if (argc < 2)
-		bad_arg = TRUE;
+		usage(stderr);
 
 	/* Parse arguments. */
-
-	for (arg = 1; arg < argc;) {
-		if (*argv[arg] == '-') {
-
-			/* Parse a single option. */
-
-			for (modifier = arg + 1; modifier < argc; modifier++) {
-				if (*argv[modifier] == '-') break;
-			}
-			parse_option(argv[arg] + 1, modifier - arg - 1,
-				     &argv[arg + 1], &bad_arg);
-			arg = modifier;
-		} else {
-			bad_arg = TRUE;
-			arg++;
-		}
-	}
-
-	/* Display syntax message if error in arguments. */
-
-	if (bad_arg)
-		usage(stderr);
+	parse_option(argc, argv);
 
 	/* Find out terminal name. */
 
-- 
1.9.2

[PATCH 03/17] setterm: use string utils to numeric parsing

[Sami Kerola]

Check the input numbers are numbers, which makes also the code shorter,
and user experience better as half invalid imputs will error.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/Makemodule.am |  2 +-
 term-utils/setterm.c     | 70 +++++++++++++++++++++++-------------------------
 2 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/term-utils/Makemodule.am b/term-utils/Makemodule.am
index e53471f..39fddc9 100644
--- a/term-utils/Makemodule.am
+++ b/term-utils/Makemodule.am
@@ -29,7 +29,7 @@ usrbin_exec_PROGRAMS += setterm
 dist_man_MANS += term-utils/setterm.1
 setterm_SOURCES = term-utils/setterm.c
 setterm_CFLAGS = $(AM_CFLAGS)
-setterm_LDADD = $(LDADD)
+setterm_LDADD = $(LDADD) libcommon.la
 if HAVE_TINFO
 setterm_LDADD += $(TINFO_LIBS)
 setterm_CFLAGS += $(TINFO_CFLAGS)
diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 25baff7..c4753d0 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -124,6 +124,7 @@
 #include "c.h"
 #include "closestream.h"
 #include "nls.h"
+#include "strutils.h"
 #include "xalloc.h"
 
 /* Constants. */
@@ -227,6 +228,15 @@ static void screendump(int vcnum, FILE *F);
  * Note that it is an error for a given option to be invoked more than once.
  */
 
+static int strtoint_or_err(const char *str)
+{
+	int64_t num = strtos64_or_err(str, _("argument error"));
+
+	if (num < INT_MIN || INT_MAX < num)
+		errx(EXIT_FAILURE, _("argument error: %s"), str);
+	return num;
+}
+
 static int parse_switch(const char *arg, const char *t, const char *f)
 {
 	if (strcmp(arg, t) == 0)
@@ -238,6 +248,8 @@ static int parse_switch(const char *arg, const char *t, const char *f)
 
 static int parse_febg_color(const char *arg)
 {
+	int color;
+
 	if (strcmp(arg, "black") == 0)
 		return BLACK;
 	else if (strcmp(arg, "red") == 0)
@@ -256,15 +268,11 @@ static int parse_febg_color(const char *arg)
 		return WHITE;
 	else if (strcmp(arg, "default") == 0)
 		return DEFAULT;
-	else if (isdigit(arg[0])) {
-		int color;
-
-		color = atoi(arg);
-		if (color < BLACK || DEFAULT < color || color == GREY)
-			errx(EXIT_FAILURE, _("argument error: %s"), arg);
-		return color;
-	}
-	errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	else
+		color = strtoint_or_err(arg);
+	if (color < BLACK || DEFAULT < color || color == GREY)
+		errx(EXIT_FAILURE, _("argument error: %s"), arg);
+	return color;
 }
 
 static int parse_ulhb_color(char **argv, int *optind)
@@ -298,11 +306,11 @@ static int parse_ulhb_color(char **argv, int *optind)
 		color = CYAN;
 	else if (strcmp(color_name, "white") == 0)
 		color = WHITE;
-	else if (isdigit(color_name[0]))
-		color = atoi(color_name);
-
-	if (color < BLACK || DEFAULT < color)
-		errx(EXIT_FAILURE, _("argument error: %s"), color_name);
+	else {
+		color = strtoint_or_err(color_name);
+		if (color < BLACK || DEFAULT < color)
+			errx(EXIT_FAILURE, _("argument error"));
+	}
 	if (bright && (color == BLACK || color == GREY))
 		errx(EXIT_FAILURE, _("argument error: bright %s is not supported"), color_name);
 
@@ -335,10 +343,9 @@ static int parse_blank(char **argv, char *optarg, int *optind)
 	else if (!strcmp(arg, "poke"))
 		return UNBLANKSCREEN;
 	else {
-		int ret = -1;
+		int ret;
 
-		if (isdigit(arg[0]))
-			ret = atoi(arg);
+		ret = strtoint_or_err(arg);
 		if (ret < 0 || BLANK_MAX < ret)
 			errx(EXIT_FAILURE, _("argument error: %s"), arg);
 		return ret;
@@ -364,10 +371,9 @@ static int parse_powersave(const char *arg)
 
 static int parse_msglevel(const char *arg)
 {
-	int ret = CONSOLE_LEVEL_MIN - 1;
+	int ret;
 
-	if (isdigit(arg[0]))
-		ret = atoi(arg);
+	ret = strtoint_or_err(arg);
 	if (ret < CONSOLE_LEVEL_MIN || CONSOLE_LEVEL_MAX < ret)
 		errx(EXIT_FAILURE, _("argument error: %s"), arg);
 	return ret;
@@ -375,14 +381,13 @@ static int parse_msglevel(const char *arg)
 
 static int parse_snap(char **argv, char *optarg, int *optind)
 {
-	int ret = 0;
+	int ret;
 	char *arg;
 
 	arg = find_optional_arg(argv, optarg, optind);
 	if (!arg)
 		return 0;
-	if (isdigit(arg[0]))
-		ret = atoi(arg);
+	ret = strtoint_or_err(arg);
 	if (ret < 1)
 		errx(EXIT_FAILURE, _("argument error: %s"), arg);
 	return ret;
@@ -393,7 +398,7 @@ static void parse_tabs(char **argv, char *optarg, int *optind, int *tab_array)
 	int i = 0;
 
 	if (optarg) {
-		tab_array[i] = atoi(optarg);
+		tab_array[i] = strtoint_or_err(optarg);
 		i++;
 	}
 	while (argv[*optind]) {
@@ -401,10 +406,7 @@ static void parse_tabs(char **argv, char *optarg, int *optind, int *tab_array)
 			errx(EXIT_FAILURE, _("too many tabs"));
 		if (argv[*optind][0] == '-')
 			break;
-		if (isdigit(argv[*optind][0]))
-			tab_array[i] = atoi(argv[*optind]);
-		else
-			break;
+		tab_array[i] = strtoint_or_err(argv[*optind]);
 		(*optind)++;
 		i++;
 	}
@@ -419,7 +421,7 @@ static int parse_regtabs(char **argv, char *optarg, int *optind)
 	arg = find_optional_arg(argv, optarg, optind);
 	if (!arg)
 		return DEFAULT_TAB_LEN;
-	ret = atoi(arg);
+	ret = strtoint_or_err(arg);
 	if (ret < 1 || TABS_MAX < ret)
 		errx(EXIT_FAILURE, _("argument error: %s"), arg);
 	return ret;
@@ -433,8 +435,7 @@ static int parse_blength(char **argv, char *optarg, int *optind)
 	arg = find_optional_arg(argv, optarg, optind);
 	if (!arg)
 		return 0;
-	if (isdigit(arg[0]))
-		ret = atoi(arg);
+	ret = strtoint_or_err(arg);
 	if (ret < 0 || BLENGTH_MAX < ret)
 		errx(EXIT_FAILURE, _("argument error: %s"), arg);
 	return ret;
@@ -447,9 +448,7 @@ static int parse_bfreq(char **argv, char *optarg, int *optind)
 	arg = find_optional_arg(argv, optarg, optind);
 	if (!arg)
 		return 0;
-	if (isdigit(arg[0]))
-		return atoi(arg);
-	return 0;
+	return strtoint_or_err(arg);
 }
 
 static void
@@ -717,8 +716,7 @@ static void parse_option(int argc, char **argv)
 			break;
 		case OPT_FILE:
 			set_opt_flag(&opt_snapfile);
-			strncpy(opt_sn_name, optarg, PATH_MAX);	/* FIXME: should use xstrncpy() */
-			opt_sn_name[PATH_MAX - 1] = 0;
+			xstrncpy(opt_sn_name, optarg, PATH_MAX);
 			break;
 		case OPT_MSG:
 			set_opt_flag(&opt_msg);
-- 
1.9.2

[PATCH 04/17] setterm: move show_tabs() and screendump() functions

[Sami Kerola]

Earlier the function was in the middle of option parsing code segment,
and screendump() required function prototype.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 187 +++++++++++++++++++++++++--------------------------
 1 file changed, 92 insertions(+), 95 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index c4753d0..734d275 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -221,8 +221,6 @@ int opt_ps_mode, opt_pd_min;	/* powersave mode/powerdown time */
 
 char opt_sn_name[PATH_MAX] = "screen.dump";
 
-static void screendump(int vcnum, FILE *F);
-
 /* Command line parsing routines.
  *
  * Note that it is an error for a given option to be invoked more than once.
@@ -451,24 +449,6 @@ static int parse_bfreq(char **argv, char *optarg, int *optind)
 	return strtoint_or_err(arg);
 }
 
-static void
-show_tabs(void) {
-	int i, co = tigetnum("cols");
-
-	if(co > 0) {
-		printf("\r         ");
-		for(i = 10; i < co-2; i+=10)
-			printf("%-10d", i);
-		putchar('\n');
-		for(i = 1; i <= co; i++)
-			putchar(i%10+'0');
-		putchar('\n');
-		for(i = 1; i < co; i++)
-			printf("\tT\b");
-		putchar('\n');
-	}
-}
-
 static void __attribute__ ((__noreturn__))
 usage(FILE *out) {
 /* Print error message about arguments, and the command's syntax. */
@@ -769,6 +749,98 @@ static char *ti_entry(const char *name) {
 	return buf_ptr;
 }
 
+static void show_tabs(void)
+{
+	int i, co = tigetnum("cols");
+
+	if (co > 0) {
+		printf("\r         ");
+		for (i = 10; i < co - 2; i += 10)
+			printf("%-10d", i);
+		putchar('\n');
+		for (i = 1; i <= co; i++)
+			putchar(i % 10 + '0');
+		putchar('\n');
+		for (i = 1; i < co; i++)
+			printf("\tT\b");
+		putchar('\n');
+	}
+}
+
+static void screendump(int vcnum, FILE *F)
+{
+	char infile[MAXPATHLEN];
+	unsigned char header[4];
+	unsigned int rows, cols;
+	int fd;
+	size_t i, j;
+	ssize_t rc;
+	char *inbuf = NULL, *outbuf = NULL, *p, *q;
+
+	sprintf(infile, "/dev/vcsa%d", vcnum);
+	fd = open(infile, O_RDONLY);
+	if (fd < 0 && vcnum == 0) {
+		/* vcsa0 is often called vcsa */
+		sprintf(infile, "/dev/vcsa");
+		fd = open(infile, O_RDONLY);
+	}
+	if (fd < 0) {
+		/* try devfs name - for zero vcnum just /dev/vcc/a */
+		/* some gcc's warn for %.u - add 0 */
+		sprintf(infile, "/dev/vcc/a%.0u", vcnum);
+		fd = open(infile, O_RDONLY);
+	}
+	if (fd < 0) {
+		sprintf(infile, "/dev/vcsa%d", vcnum);
+		goto read_error;
+	}
+	if (read(fd, header, 4) != 4)
+		goto read_error;
+	rows = header[0];
+	cols = header[1];
+	if (rows * cols == 0)
+		goto read_error;
+
+	inbuf = xmalloc(rows * cols * 2);
+	outbuf = xmalloc(rows * (cols + 1));
+
+	rc = read(fd, inbuf, rows * cols * 2);
+	if (rc < 0 || (size_t)rc != rows * cols * 2)
+		goto read_error;
+	p = inbuf;
+	q = outbuf;
+	for (i = 0; i < rows; i++) {
+		for (j = 0; j < cols; j++) {
+			*q++ = *p;
+			p += 2;
+		}
+		while (j-- > 0 && q[-1] == ' ')
+			q--;
+		*q++ = '\n';
+	}
+	if (fwrite(outbuf, 1, q - outbuf, F) != (size_t)(q - outbuf)) {
+		warnx(_("Error writing screendump"));
+		goto error;
+	}
+	close(fd);
+	free(inbuf);
+	free(outbuf);
+	return;
+
+ read_error:
+	if (vcnum != 0)
+		warnx(_("Couldn't read %s"), infile);
+	else
+		warnx(_("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
+
+ error:
+	if (fd >= 0)
+		close(fd);
+	free(inbuf);
+	free(outbuf);
+	exit(EXIT_FAILURE);
+}
+
 static void
 perform_sequence(int vcterm) {
 	/* vcterm: Set if terminal is a virtual console. */
@@ -1033,81 +1105,6 @@ perform_sequence(int vcterm) {
 
 }
 
-static void
-screendump(int vcnum, FILE * F)
-{
-	char infile[MAXPATHLEN];
-	unsigned char header[4];
-	unsigned int rows, cols;
-	int fd;
-	size_t i, j;
-	ssize_t rc;
-	char *inbuf = NULL, *outbuf = NULL, *p, *q;
-
-	sprintf(infile, "/dev/vcsa%d", vcnum);
-	fd = open(infile, O_RDONLY);
-	if (fd < 0 && vcnum == 0) {
-		/* vcsa0 is often called vcsa */
-		sprintf(infile, "/dev/vcsa");
-		fd = open(infile, O_RDONLY);
-	}
-	if (fd < 0) {
-		/* try devfs name - for zero vcnum just /dev/vcc/a */
-		/* some gcc's warn for %.u - add 0 */
-		sprintf(infile, "/dev/vcc/a%.0u", vcnum);
-		fd = open(infile, O_RDONLY);
-	}
-	if (fd < 0) {
-		sprintf(infile, "/dev/vcsa%d", vcnum);
-		goto read_error;
-	}
-	if (read(fd, header, 4) != 4)
-		goto read_error;
-	rows = header[0];
-	cols = header[1];
-	if (rows * cols == 0)
-		goto read_error;
-
-	inbuf = xmalloc(rows * cols * 2);
-	outbuf = xmalloc(rows * (cols + 1));
-
-	rc = read(fd, inbuf, rows * cols * 2);
-	if (rc < 0 || (size_t) rc != rows * cols * 2)
-		goto read_error;
-	p = inbuf;
-	q = outbuf;
-	for (i = 0; i < rows; i++) {
-		for (j = 0; j < cols; j++) {
-			*q++ = *p;
-			p += 2;
-		}
-		while (j-- > 0 && q[-1] == ' ')
-			q--;
-		*q++ = '\n';
-	}
-	if (fwrite(outbuf, 1, q - outbuf, F) != (size_t) (q - outbuf)) {
-		warnx(_("Error writing screendump"));
-		goto error;
-	}
-	close(fd);
-	free(inbuf);
-	free(outbuf);
-	return;
-
-read_error:
-	if (vcnum != 0)
-		warnx(_("Couldn't read %s"), infile);
-	else
-		warnx(_("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
-
-error:
-	if (fd >= 0)
-		close(fd);
-	free(inbuf);
-	free(outbuf);
-	exit(EXIT_FAILURE);
-}
-
 int
 main(int argc, char **argv) {
 	char *term;			/* Terminal type. */
-- 
1.9.2

[PATCH 05/17] setterm: remove usage comment segment

[Sami Kerola]

It is enough trouble to keep usage output aligned with getopt() parsing,
manual page, and bash-completion, so get rid of the unnecessary effort
duplication.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 46 ----------------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 734d275..268ddec 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -19,52 +19,6 @@
  * 1999-02-22 Arkadiusz Miśkiewicz <misiek@pld.ORG.PL>
  * - added Native Language Support
  *
- *
- * Syntax:
- *
- * setterm
- *   [ -term terminal_name ]
- *   [ -reset ]
- *   [ -initialize ]
- *   [ -cursor [on|off] ]
- *   [ -repeat [on|off] ]
- *   [ -appcursorkeys [on|off] ]
- *   [ -linewrap [on|off] ]
- *   [ -snow [on|off] ]
- *   [ -softscroll [on|off] ]
- *   [ -defaults ]
- *   [ -foreground black|red|green|yellow|blue|magenta|cyan|white|default ]
- *   [ -background black|red|green|yellow|blue|magenta|cyan|white|default ]
- *   [ -ulcolor black|grey|red|green|yellow|blue|magenta|cyan|white ]
- *   [ -ulcolor bright red|green|yellow|blue|magenta|cyan|white ]
- *   [ -hbcolor black|grey|red|green|yellow|blue|magenta|cyan|white ]
- *   [ -hbcolor bright red|green|yellow|blue|magenta|cyan|white ]
- *   [ -inversescreen [on|off] ]
- *   [ -bold [on|off] ]
- *   [ -half-bright [on|off] ]
- *   [ -blink [on|off] ]
- *   [ -reverse [on|off] ]
- *   [ -underline [on|off] ]
- *   [ -store ]
- *   [ -clear [ all|rest ] ]
- *   [ -tabs [tab1 tab2 tab3 ... ] ]     (tabn = 1-160)
- *   [ -clrtabs [ tab1 tab2 tab3 ... ]   (tabn = 1-160)
- *   [ -regtabs [1-160] ]
- *   [ -blank [0-60|force|poke|] ]
- *   [ -dump   [1-NR_CONS ] ]
- *   [ -append [1-NR_CONS ] ]
- *   [ -file dumpfilename ]
- *   [ -standout [attr] ]
- *   [ -msg [on|off] ]
- *   [ -msglevel [0-8] ]
- *   [ -powersave [on|vsync|hsync|powerdown|off] ]
- *   [ -powerdown [0-60] ]
- *   [ -blength [0-2000] ]
- *   [ -bfreq freq ]
- *   [ -version ]
- *   [ -help ]
- *
- *
  * Semantics:
  *
  * Setterm writes to standard output a character string that will
-- 
1.9.2

[PATCH 06/17] setterm: add option control structure

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 394 +++++++++++++++++++++++++--------------------------
 1 file changed, 195 insertions(+), 199 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 268ddec..63c912b 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -144,36 +144,36 @@ enum {
 #define	TABS_MAX	160
 #define BLENGTH_MAX	2000
 
-/* Option flags.  Set if the option is to be invoked. */
-int opt_term, opt_reset, opt_initialize, opt_cursor;
-int opt_linewrap, opt_default, opt_foreground;
-int opt_background, opt_bold, opt_blink, opt_reverse, opt_underline;
-int opt_store, opt_clear, opt_blank, opt_snap, opt_snapfile;
-int opt_append, opt_ulcolor, opt_hbcolor, opt_halfbright, opt_repeat;
-int opt_tabs, opt_clrtabs, opt_regtabs, opt_appcursorkeys, opt_inversescreen;
-int opt_msg, opt_msglevel, opt_powersave, opt_powerdown;
-int opt_blength, opt_bfreq;
-
-/* Option controls.  The variable names have been contracted to ensure
- * uniqueness.
- */
-char *opt_te_terminal_name;	/* Terminal name. */
-int opt_cu_on, opt_li_on, opt_bo_on, opt_hb_on, opt_bl_on;
-int opt_re_on, opt_un_on, opt_rep_on, opt_appck_on, opt_invsc_on;
-int opt_msg_on;			/* Boolean switches. */
-int opt_fo_color, opt_ba_color;	/* Colors. */
-int opt_ul_color, opt_hb_color;
-int opt_cl_all;			/* Clear all or rest. */
-int opt_bl_min;			/* Blank screen. */
-int opt_blength_l;
-int opt_bfreq_f;
-int opt_sn_num;			/* Snap screen. */
-int opt_rt_len;			/* regular tab length */
-int opt_tb_array[TABS_MAX + 1];	/* Array for tab list */
-int opt_msglevel_num;
-int opt_ps_mode, opt_pd_min;	/* powersave mode/powerdown time */
-
-char opt_sn_name[PATH_MAX] = "screen.dump";
+/* Option controls. */
+struct setterm_control {
+	char *opt_te_terminal_name;	/* terminal name */
+	int opt_bl_min;		/* blank screen */
+	int opt_blength_l;	/* bell duration in milliseconds */
+	int opt_bfreq_f;	/* bell frequency in Hz */
+	int opt_sn_num;		/* console number to be snapshoted */
+	char *opt_sn_name;	/* path to write snap */
+	int opt_msglevel_num;	/* printk() loging level */
+	int opt_ps_mode;	/* powersave mode */
+	int opt_pd_min;		/* powerdown time */
+	int opt_rt_len;		/* regular tab length */
+	int opt_tb_array[TABS_MAX + 1];	/* array for tab list */
+	/* colors */
+	int opt_fo_color:4, opt_ba_color:4, opt_ul_color:4, opt_hb_color:4;
+	/* boolean options */
+	int opt_cu_on:1, opt_li_on:1, opt_bo_on:1, opt_hb_on:1, opt_bl_on:1,
+	    opt_re_on:1, opt_un_on:1, opt_rep_on:1, opt_appck_on:1,
+	    opt_invsc_on:1, opt_msg_on:1, opt_cl_all:1, vcterm:1;
+	/* Option flags.  Set when an option is invoked. */
+	uint64_t opt_term:1, opt_reset:1, opt_initialize:1, opt_cursor:1,
+	    opt_linewrap:1, opt_default:1, opt_foreground:1,
+	    opt_background:1, opt_bold:1, opt_blink:1, opt_reverse:1,
+	    opt_underline:1, opt_store:1, opt_clear:1, opt_blank:1,
+	    opt_snap:1, opt_snapfile:1, opt_append:1, opt_ulcolor:1,
+	    opt_hbcolor:1, opt_halfbright:1, opt_repeat:1, opt_tabs:1,
+	    opt_clrtabs:1, opt_regtabs:1, opt_appcursorkeys:1,
+	    opt_inversescreen:1, opt_msg:1, opt_msglevel:1, opt_powersave:1,
+	    opt_powerdown:1, opt_blength:1, opt_bfreq:1;
+};
 
 /* Command line parsing routines.
  *
@@ -458,14 +458,14 @@ usage(FILE *out) {
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static void set_opt_flag(int *opt)
+static int __attribute__((__pure__)) set_opt_flag(int opt)
 {
-	if (*opt)
+	if (opt)
 		errx(EXIT_FAILURE, "duplicate use of an option");
-	*opt = 1;
+	return 1;
 }
 
-static void parse_option(int argc, char **argv)
+static void parse_option(struct setterm_control *ctl, int argc, char **argv)
 {
 	int c;
 	enum {
@@ -549,132 +549,132 @@ static void parse_option(int argc, char **argv)
 	while ((c = getopt_long_only(argc, argv, "", longopts, NULL)) != -1) {
 		switch (c) {
 		case OPT_TERM:
-			set_opt_flag(&opt_term);
-			opt_te_terminal_name = optarg;
+			ctl->opt_term = set_opt_flag(ctl->opt_term);
+			ctl->opt_te_terminal_name = optarg;
 			break;
 		case OPT_RESET:
-			set_opt_flag(&opt_reset);
+			ctl->opt_reset = set_opt_flag(ctl->opt_reset);
 			break;
 		case OPT_INITIALIZE:
-			set_opt_flag(&opt_initialize);
+			ctl->opt_initialize = set_opt_flag(ctl->opt_initialize);
 			break;
 		case OPT_CURSOR:
-			set_opt_flag(&opt_cursor);
-			opt_cu_on = parse_switch(optarg, "on", "off");
+			ctl->opt_cursor = set_opt_flag(ctl->opt_cursor);
+			ctl->opt_cu_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_REPEAT:
-			set_opt_flag(&opt_repeat);
-			opt_rep_on = parse_switch(optarg, "on", "off");
+			ctl->opt_repeat = set_opt_flag(ctl->opt_repeat);
+			ctl->opt_rep_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_APPCURSORKEYS:
-			set_opt_flag(&opt_appcursorkeys);
-			opt_appck_on = parse_switch(optarg, "on", "off");
+			ctl->opt_appcursorkeys = set_opt_flag(ctl->opt_appcursorkeys);
+			ctl->opt_appck_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_LINEWRAP:
-			set_opt_flag(&opt_linewrap);
-			opt_li_on = parse_switch(optarg, "on", "off");
+			ctl->opt_linewrap = set_opt_flag(ctl->opt_linewrap);
+			ctl->opt_li_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_DEFAULT:
-			set_opt_flag(&opt_default);
+			ctl->opt_default = set_opt_flag(ctl->opt_default);
 			break;
 		case OPT_FOREGROUND:
-			set_opt_flag(&opt_foreground);
-			opt_fo_color = parse_febg_color(optarg);
+			ctl->opt_foreground = set_opt_flag(ctl->opt_foreground);
+			ctl->opt_fo_color = parse_febg_color(optarg);
 			break;
 		case OPT_BACKGROUND:
-			set_opt_flag(&opt_background);
-			opt_ba_color = parse_febg_color(optarg);
+			ctl->opt_background = set_opt_flag(ctl->opt_background);
+			ctl->opt_ba_color = parse_febg_color(optarg);
 			break;
 		case OPT_ULCOLOR:
-			set_opt_flag(&opt_ulcolor);
-			opt_ul_color = parse_ulhb_color(argv, &optind);
+			ctl->opt_ulcolor = set_opt_flag(ctl->opt_ulcolor);
+			ctl->opt_ul_color = parse_ulhb_color(argv, &optind);
 			break;
 		case OPT_HBCOLOR:
-			set_opt_flag(&opt_hbcolor);
-			opt_hb_color = parse_ulhb_color(argv, &optind);
+			ctl->opt_hbcolor = set_opt_flag(ctl->opt_hbcolor);
+			ctl->opt_hb_color = parse_ulhb_color(argv, &optind);
 			break;
 		case OPT_INVERSESCREEN:
-			set_opt_flag(&opt_inversescreen);
-			opt_invsc_on = parse_switch(optarg, "on", "off");
+			ctl->opt_inversescreen = set_opt_flag(ctl->opt_inversescreen);
+			ctl->opt_invsc_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_BOLD:
-			set_opt_flag(&opt_bold);
-			opt_bo_on = parse_switch(optarg, "on", "off");
+			ctl->opt_bold = set_opt_flag(ctl->opt_bold);
+			ctl->opt_bo_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_HALF_BRIGHT:
-			set_opt_flag(&opt_halfbright);
-			opt_hb_on = parse_switch(optarg, "on", "off");
+			ctl->opt_halfbright = set_opt_flag(ctl->opt_halfbright);
+			ctl->opt_hb_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_BLINK:
-			set_opt_flag(&opt_blink);
-			opt_bl_on = parse_switch(optarg, "on", "off");
+			ctl->opt_blink = set_opt_flag(ctl->opt_blink);
+			ctl->opt_bl_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_REVERSE:
-			set_opt_flag(&opt_reverse);
-			opt_re_on = parse_switch(optarg, "on", "off");
+			ctl->opt_reverse = set_opt_flag(ctl->opt_reverse);
+			ctl->opt_re_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_UNDERLINE:
-			set_opt_flag(&opt_underline);
-			opt_un_on = parse_switch(optarg, "on", "off");
+			ctl->opt_underline = set_opt_flag(ctl->opt_underline);
+			ctl->opt_un_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_STORE:
-			set_opt_flag(&opt_store);
+			ctl->opt_store = set_opt_flag(ctl->opt_store);
 			break;
 		case OPT_CLEAR:
-			set_opt_flag(&opt_clear);
-			opt_cl_all = parse_switch(optarg, "all", "reset");
+			ctl->opt_clear = set_opt_flag(ctl->opt_clear);
+			ctl->opt_cl_all = parse_switch(optarg, "all", "reset");
 			break;
 		case OPT_TABS:
-			set_opt_flag(&opt_tabs);
-			parse_tabs(argv, optarg, &optind, opt_tb_array);
+			ctl->opt_tabs = set_opt_flag(ctl->opt_tabs);
+			parse_tabs(argv, optarg, &optind, ctl->opt_tb_array);
 			break;
 		case OPT_CLRTABS:
-			set_opt_flag(&opt_clrtabs);
-			parse_tabs(argv, optarg, &optind, opt_tb_array);
+			ctl->opt_clrtabs = set_opt_flag(ctl->opt_clrtabs);
+			parse_tabs(argv, optarg, &optind, ctl->opt_tb_array);
 			break;
 		case OPT_REGTABS:
-			set_opt_flag(&opt_regtabs);
-			opt_rt_len = parse_regtabs(argv, optarg, &optind);
+			ctl->opt_regtabs = set_opt_flag(ctl->opt_regtabs);
+			ctl->opt_rt_len = parse_regtabs(argv, optarg, &optind);
 			break;
 		case OPT_BLANK:
-			set_opt_flag(&opt_blank);
-			opt_bl_min = parse_blank(argv, optarg, &optind);
+			ctl->opt_blank = set_opt_flag(ctl->opt_blank);
+			ctl->opt_bl_min = parse_blank(argv, optarg, &optind);
 			break;
 		case OPT_DUMP:
-			set_opt_flag(&opt_snap);
-			opt_sn_num = parse_snap(argv, optarg, &optind);
+			ctl->opt_snap = set_opt_flag(ctl->opt_snap);
+			ctl->opt_sn_num = parse_snap(argv, optarg, &optind);
 			break;
 		case OPT_APPEND:
-			set_opt_flag(&opt_append);
-			opt_sn_num = parse_snap(argv, optarg, &optind);
+			ctl->opt_append = set_opt_flag(ctl->opt_append);
+			ctl->opt_sn_num = parse_snap(argv, optarg, &optind);
 			break;
 		case OPT_FILE:
-			set_opt_flag(&opt_snapfile);
-			xstrncpy(opt_sn_name, optarg, PATH_MAX);
+			ctl->opt_snapfile = set_opt_flag(ctl->opt_snapfile);
+			ctl->opt_sn_name = optarg;
 			break;
 		case OPT_MSG:
-			set_opt_flag(&opt_msg);
-			opt_msg_on = parse_switch(optarg, "on", "off");
+			ctl->opt_msg = set_opt_flag(ctl->opt_msg);
+			ctl->opt_msg_on = parse_switch(optarg, "on", "off");
 			break;
 		case OPT_MSGLEVEL:
-			set_opt_flag(&opt_msglevel);
-			opt_msglevel_num = parse_msglevel(optarg);
+			ctl->opt_msglevel = set_opt_flag(ctl->opt_msglevel);
+			ctl->opt_msglevel_num = parse_msglevel(optarg);
 			break;
 		case OPT_POWERSAVE:
-			set_opt_flag(&opt_powersave);
-			opt_ps_mode = parse_powersave(optarg);
+			ctl->opt_powersave = set_opt_flag(ctl->opt_powersave);
+			ctl->opt_ps_mode = parse_powersave(optarg);
 			break;
 		case OPT_POWERDOWN:
-			set_opt_flag(&opt_powerdown);
-			opt_pd_min = parse_blank(argv, optarg, &optind);
+			ctl->opt_powerdown = set_opt_flag(ctl->opt_powerdown);
+			ctl->opt_pd_min = parse_blank(argv, optarg, &optind);
 			break;
 		case OPT_BLENGTH:
-			set_opt_flag(&opt_blength);
-			opt_blength_l = parse_blength(argv, optarg, &optind);
+			ctl->opt_blength = set_opt_flag(ctl->opt_blength);
+			ctl->opt_blength_l = parse_blength(argv, optarg, &optind);
 			break;
 		case OPT_BFREQ:
-			set_opt_flag(&opt_bfreq);
-			opt_bfreq_f = parse_bfreq(argv, optarg, &optind);
+			ctl->opt_bfreq = set_opt_flag(ctl->opt_bfreq);
+			ctl->opt_bfreq_f = parse_bfreq(argv, optarg, &optind);
 			break;
 		case OPT_VERSION:
 			printf(UTIL_LINUX_VERSION);
@@ -721,7 +721,7 @@ static void show_tabs(void)
 	}
 }
 
-static void screendump(int vcnum, FILE *F)
+static void screendump(struct setterm_control *ctl, FILE *F)
 {
 	char infile[MAXPATHLEN];
 	unsigned char header[4];
@@ -731,21 +731,21 @@ static void screendump(int vcnum, FILE *F)
 	ssize_t rc;
 	char *inbuf = NULL, *outbuf = NULL, *p, *q;
 
-	sprintf(infile, "/dev/vcsa%d", vcnum);
+	sprintf(infile, "/dev/vcsa%d", ctl->opt_sn_num);
 	fd = open(infile, O_RDONLY);
-	if (fd < 0 && vcnum == 0) {
+	if (fd < 0 && ctl->opt_sn_num == 0) {
 		/* vcsa0 is often called vcsa */
 		sprintf(infile, "/dev/vcsa");
 		fd = open(infile, O_RDONLY);
 	}
 	if (fd < 0) {
-		/* try devfs name - for zero vcnum just /dev/vcc/a */
+		/* try devfs name - for zero ctl->opt_sn_num just /dev/vcc/a */
 		/* some gcc's warn for %.u - add 0 */
-		sprintf(infile, "/dev/vcc/a%.0u", vcnum);
+		sprintf(infile, "/dev/vcc/a%.0u", ctl->opt_sn_num);
 		fd = open(infile, O_RDONLY);
 	}
 	if (fd < 0) {
-		sprintf(infile, "/dev/vcsa%d", vcnum);
+		sprintf(infile, "/dev/vcsa%d", ctl->opt_sn_num);
 		goto read_error;
 	}
 	if (read(fd, header, 4) != 4)
@@ -782,7 +782,7 @@ static void screendump(int vcnum, FILE *F)
 	return;
 
  read_error:
-	if (vcnum != 0)
+	if (ctl->opt_sn_num != 0)
 		warnx(_("Couldn't read %s"), infile);
 	else
 		warnx(_("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
@@ -795,48 +795,46 @@ static void screendump(int vcnum, FILE *F)
 	exit(EXIT_FAILURE);
 }
 
-static void
-perform_sequence(int vcterm) {
-	/* vcterm: Set if terminal is a virtual console. */
-
+static void perform_sequence(struct setterm_control *ctl)
+{
 	int result;
 /* Perform the selected options. */
 
 	/* -reset. */
-	if (opt_reset) {
+	if (ctl->opt_reset) {
 		putp(ti_entry("rs1"));
 	}
 
 	/* -initialize. */
-	if (opt_initialize) {
+	if (ctl->opt_initialize) {
 		putp(ti_entry("is2"));
 	}
 
 	/* -cursor [on|off]. */
-	if (opt_cursor) {
-		if (opt_cu_on)
+	if (ctl->opt_cursor) {
+		if (ctl->opt_cu_on)
 			putp(ti_entry("cnorm"));
 		else
 			putp(ti_entry("civis"));
 	}
 
 	/* -linewrap [on|off]. Vc only (vt102) */
-	if (opt_linewrap && vcterm)
-		fputs(opt_li_on ? "\033[?7h" : "\033[?7l", stdout);
+	if (ctl->opt_linewrap && ctl->vcterm)
+		fputs(ctl->opt_li_on ? "\033[?7h" : "\033[?7l", stdout);
 
 	/* -repeat [on|off]. Vc only (vt102) */
-	if (opt_repeat && vcterm)
-		fputs(opt_rep_on ? "\033[?8h" : "\033[?8l", stdout);
+	if (ctl->opt_repeat && ctl->vcterm)
+		fputs(ctl->opt_rep_on ? "\033[?8h" : "\033[?8l", stdout);
 
 	/* -appcursorkeys [on|off]. Vc only (vt102) */
-	if (opt_appcursorkeys && vcterm)
-		fputs(opt_appck_on ? "\033[?1h" : "\033[?1l", stdout);
+	if (ctl->opt_appcursorkeys && ctl->vcterm)
+		fputs(ctl->opt_appck_on ? "\033[?1h" : "\033[?1l", stdout);
 
 	/* -default.  Vc sets default rendition, otherwise clears all
 	 * attributes.
 	 */
-	if (opt_default) {
-		if (vcterm)
+	if (ctl->opt_default) {
+		if (ctl->vcterm)
 			printf("\033[0m");
 		else
 			putp(ti_entry("sgr0"));
@@ -845,42 +843,42 @@ perform_sequence(int vcterm) {
 	/* -foreground black|red|green|yellow|blue|magenta|cyan|white|default.
 	 * Vc only (ANSI).
 	 */
-	if (opt_foreground && vcterm)
-		printf("\033[3%c%s", '0' + opt_fo_color, "m");
+	if (ctl->opt_foreground && ctl->vcterm)
+		printf("\033[3%c%s", '0' + ctl->opt_fo_color, "m");
 
 	/* -background black|red|green|yellow|blue|magenta|cyan|white|default.
 	 * Vc only (ANSI).
 	 */
-	if (opt_background && vcterm)
-		printf("\033[4%c%s", '0' + opt_ba_color, "m");
+	if (ctl->opt_background && ctl->vcterm)
+		printf("\033[4%c%s", '0' + ctl->opt_ba_color, "m");
 
 	/* -ulcolor black|red|green|yellow|blue|magenta|cyan|white|default.
 	 * Vc only.
 	 */
-	if (opt_ulcolor && vcterm) {
-		printf("\033[1;%d]", opt_ul_color);
+	if (ctl->opt_ulcolor && ctl->vcterm) {
+		printf("\033[1;%d]", ctl->opt_ul_color);
 	}
 
 	/* -hbcolor black|red|green|yellow|blue|magenta|cyan|white|default.
 	 * Vc only.
 	 */
-	if (opt_hbcolor && vcterm) {
-		printf("\033[2;%d]", opt_hb_color);
+	if (ctl->opt_hbcolor && ctl->vcterm) {
+		printf("\033[2;%d]", ctl->opt_hb_color);
 	}
 
 	/* -inversescreen [on|off].  Vc only (vt102).
 	 */
-	if (opt_inversescreen && vcterm)
-		fputs(opt_invsc_on ? "\033[?5h" : "\033[?5l", stdout);
+	if (ctl->opt_inversescreen && ctl->vcterm)
+		fputs(ctl->opt_invsc_on ? "\033[?5h" : "\033[?5l", stdout);
 
 	/* -bold [on|off].  Vc behaves as expected, otherwise off turns off
 	 * all attributes.
 	 */
-	if (opt_bold) {
-		if (opt_bo_on)
+	if (ctl->opt_bold) {
+		if (ctl->opt_bo_on)
 			putp(ti_entry("bold"));
 		else {
-			if (vcterm)
+			if (ctl->vcterm)
 				fputs("\033[22m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -890,11 +888,11 @@ perform_sequence(int vcterm) {
 	/* -half-bright [on|off].  Vc behaves as expected, otherwise off turns off
 	 * all attributes.
 	 */
-	if (opt_halfbright) {
-		if (opt_hb_on)
+	if (ctl->opt_halfbright) {
+		if (ctl->opt_hb_on)
 			putp(ti_entry("dim"));
 		else {
-			if (vcterm)
+			if (ctl->vcterm)
 				fputs("\033[22m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -904,11 +902,11 @@ perform_sequence(int vcterm) {
 	/* -blink [on|off].  Vc behaves as expected, otherwise off turns off
 	 * all attributes.
 	 */
-	if (opt_blink) {
-		if (opt_bl_on)
+	if (ctl->opt_blink) {
+		if (ctl->opt_bl_on)
 			putp(ti_entry("blink"));
 		else {
-			if (vcterm)
+			if (ctl->vcterm)
 				fputs("\033[25m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -918,11 +916,11 @@ perform_sequence(int vcterm) {
 	/* -reverse [on|off].  Vc behaves as expected, otherwise off turns
 	 * off all attributes.
 	 */
-	if (opt_reverse) {
-		if (opt_re_on)
+	if (ctl->opt_reverse) {
+		if (ctl->opt_re_on)
 			putp(ti_entry("rev"));
 		else {
-			if (vcterm)
+			if (ctl->vcterm)
 				fputs("\033[27m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -930,65 +928,65 @@ perform_sequence(int vcterm) {
 	}
 
 	/* -underline [on|off]. */
-	if (opt_underline)
-		putp(ti_entry(opt_un_on ? "smul" : "rmul"));
+	if (ctl->opt_underline)
+		putp(ti_entry(ctl->opt_un_on ? "smul" : "rmul"));
 
 	/* -store.  Vc only. */
-	if (opt_store && vcterm)
+	if (ctl->opt_store && ctl->vcterm)
 		fputs("\033[8]", stdout);
 
 	/* -clear [all|rest]. */
-	if (opt_clear)
-		putp(ti_entry(opt_cl_all ? "clear" : "ed"));
+	if (ctl->opt_clear)
+		putp(ti_entry(ctl->opt_cl_all ? "clear" : "ed"));
 
 	/* -tabs Vc only. */
-	if (opt_tabs && vcterm) {
+	if (ctl->opt_tabs && ctl->vcterm) {
 		int i;
 
-		if (opt_tb_array[0] == -1)
+		if (ctl->opt_tb_array[0] == -1)
 			show_tabs();
 		else {
-			for(i=0; opt_tb_array[i] > 0; i++)
-				printf("\033[%dG\033H", opt_tb_array[i]);
+			for(i=0; ctl->opt_tb_array[i] > 0; i++)
+				printf("\033[%dG\033H", ctl->opt_tb_array[i]);
 			putchar('\r');
 		}
 	}
 
 	/* -clrtabs Vc only. */
-	if (opt_clrtabs && vcterm) {
+	if (ctl->opt_clrtabs && ctl->vcterm) {
 		int i;
 
-		if (opt_tb_array[0] == -1)
+		if (ctl->opt_tb_array[0] == -1)
 			fputs("\033[3g", stdout);
 		else
-			for(i=0; opt_tb_array[i] > 0; i++)
-				printf("\033[%dG\033[g", opt_tb_array[i]);
+			for(i=0; ctl->opt_tb_array[i] > 0; i++)
+				printf("\033[%dG\033[g", ctl->opt_tb_array[i]);
 		putchar('\r');
 	}
 
 	/* -regtabs Vc only. */
-	if (opt_regtabs && vcterm) {
+	if (ctl->opt_regtabs && ctl->vcterm) {
 		int i;
 
 		fputs("\033[3g\r", stdout);
-		for(i=opt_rt_len+1; i<=TABS_MAX; i+=opt_rt_len)
-			printf("\033[%dC\033H",opt_rt_len);
+		for(i=ctl->opt_rt_len+1; i<=TABS_MAX; i+=ctl->opt_rt_len)
+			printf("\033[%dC\033H",ctl->opt_rt_len);
 		putchar('\r');
 	}
 
 	/* -blank [0-60]. */
-	if (opt_blank && vcterm) {
-		if (opt_bl_min >= 0)
-			printf("\033[9;%d]", opt_bl_min);
-		else if (opt_bl_min == BLANKSCREEN) {
+	if (ctl->opt_blank && ctl->vcterm) {
+		if (ctl->opt_bl_min >= 0)
+			printf("\033[9;%d]", ctl->opt_bl_min);
+		else if (ctl->opt_bl_min == BLANKSCREEN) {
 			char ioctlarg = TIOCL_BLANKSCREEN;
 			if (ioctl(0,TIOCLINUX,&ioctlarg))
 				warn(_("cannot force blank"));
-		} else if (opt_bl_min == UNBLANKSCREEN) {
+		} else if (ctl->opt_bl_min == UNBLANKSCREEN) {
 			char ioctlarg = TIOCL_UNBLANKSCREEN;
 			if (ioctl(0,TIOCLINUX,&ioctlarg))
 				warn(_("cannot force unblank"));
-		} else if (opt_bl_min == BLANKEDSCREEN) {
+		} else if (ctl->opt_bl_min == BLANKEDSCREEN) {
 			char ioctlarg = TIOCL_BLANKEDSCREEN;
 			int ret;
 			ret = ioctl(0,TIOCLINUX,&ioctlarg);
@@ -1000,35 +998,38 @@ perform_sequence(int vcterm) {
 	}
 
 	/* -powersave [on|vsync|hsync|powerdown|off] (console) */
-	if (opt_powersave) {
+	if (ctl->opt_powersave) {
 		char ioctlarg[2];
 		ioctlarg[0] = TIOCL_SETVESABLANK;
-		ioctlarg[1] = opt_ps_mode;
+		ioctlarg[1] = ctl->opt_ps_mode;
 		if (ioctl(0,TIOCLINUX,ioctlarg))
 			warn(_("cannot (un)set powersave mode"));
 	}
 
 	/* -powerdown [0-60]. */
-	if (opt_powerdown) {
-		printf("\033[14;%d]", opt_pd_min);
+	if (ctl->opt_powerdown) {
+		printf("\033[14;%d]", ctl->opt_pd_min);
 	}
 
 	/* -snap [1-NR_CONS]. */
-	if (opt_snap || opt_append) {
+	if (ctl->opt_snap || ctl->opt_append) {
 		FILE *F;
+		char dump_default[] = "screen.dump";
 
-		F = fopen(opt_sn_name, opt_snap ? "w" : "a");
+		if (!ctl->opt_sn_name)
+			ctl->opt_sn_name = dump_default;
+		F = fopen(ctl->opt_sn_name, ctl->opt_snap ? "w" : "a");
 		if (!F)
 			err(EXIT_DUMPFILE, _("can not open dump file %s for output"),
-				opt_sn_name); 
-		screendump(opt_sn_num, F);
+				ctl->opt_sn_name);
+		screendump(ctl, F);
 		if (close_stream(F) != 0)
 			errx(EXIT_FAILURE, _("write error"));
 	}
 
 	/* -msg [on|off]. */
-	if (opt_msg && vcterm) {
-		if (opt_msg_on)
+	if (ctl->opt_msg && ctl->vcterm) {
+		if (ctl->opt_msg_on)
 			/* 7 -- Enable printk's to console */
 			result = klogctl(SYSLOG_ACTION_CONSOLE_ON, NULL, 0);
 		else
@@ -1040,31 +1041,31 @@ perform_sequence(int vcterm) {
 	}
 
 	/* -msglevel [0-8] */
-	if (opt_msglevel && vcterm) {
+	if (ctl->opt_msglevel && ctl->vcterm) {
 		/* 8 -- Set level of messages printed to console */
-		result = klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL, opt_msglevel_num);
+		result = klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL, ctl->opt_msglevel_num);
 		if (result != 0)
 			warn(_("klogctl error"));
 	}
 
 	/* -blength [0-2000] */
-	if (opt_blength && vcterm) {
-		printf("\033[11;%d]", opt_blength_l);
+	if (ctl->opt_blength && ctl->vcterm) {
+		printf("\033[11;%d]", ctl->opt_blength_l);
 	}
 
 	/* -bfreq freqnumber */
-	if (opt_bfreq && vcterm) {
-		printf("\033[10;%d]", opt_bfreq_f);
+	if (ctl->opt_bfreq && ctl->vcterm) {
+		printf("\033[10;%d]", ctl->opt_bfreq_f);
 	}
 
 }
 
-int
-main(int argc, char **argv) {
-	char *term;			/* Terminal type. */
-	int vcterm;			/* Set if terminal is a virtual console. */
-	int errret;
+int main(int argc, char **argv)
+{
+	struct setterm_control ctl;
+	int term_errno;
 
+	memset(&ctl, 0, sizeof (ctl));
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
@@ -1074,37 +1075,32 @@ main(int argc, char **argv) {
 		usage(stderr);
 
 	/* Parse arguments. */
-	parse_option(argc, argv);
+	parse_option(&ctl, argc, argv);
 
 	/* Find out terminal name. */
-
-	if (opt_term) {
-		term = opt_te_terminal_name;
-	} else {
-		term = getenv("TERM");
-		if (term == NULL)
+	if (!ctl.opt_te_terminal_name) {
+		ctl.opt_te_terminal_name = getenv("TERM");
+		if (ctl.opt_te_terminal_name == NULL)
 			errx(EXIT_FAILURE, _("$TERM is not defined."));
 	}
 
 	/* Find terminfo entry. */
-
-	if (setupterm(term, 1, &errret))
-		switch(errret) {
+	if (setupterm(ctl.opt_te_terminal_name, 1, &term_errno))
+		switch(term_errno) {
 		case -1:
 			errx(EXIT_FAILURE, _("terminfo database cannot be found"));
 		case 0:
-			errx(EXIT_FAILURE, _("%s: unknown terminal type"), term);
+			errx(EXIT_FAILURE, _("%s: unknown terminal type"), ctl.opt_te_terminal_name);
 		case 1:
 			errx(EXIT_FAILURE, _("terminal is hardcopy"));
 		}
 
 	/* See if the terminal is a virtual console terminal. */
-
-	vcterm = (!strncmp(term, "con", 3) || !strncmp(term, "linux", 5));
+	ctl.vcterm = (!strncmp(ctl.opt_te_terminal_name, "con", 3) ||
+		      !strncmp(ctl.opt_te_terminal_name, "linux", 5));
 
 	/* Perform the selected options. */
-
-	perform_sequence(vcterm);
+	perform_sequence(&ctl);
 
 	return EXIT_SUCCESS;
 }
-- 
1.9.2

[PATCH 07/17] setterm: add init_terminal() to make main() shorter

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 63c912b..05ea387 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -1060,46 +1060,48 @@ static void perform_sequence(struct setterm_control *ctl)
 
 }
 
-int main(int argc, char **argv)
+static void init_terminal(struct setterm_control *ctl)
 {
-	struct setterm_control ctl;
 	int term_errno;
 
-	memset(&ctl, 0, sizeof (ctl));
-	setlocale(LC_ALL, "");
-	bindtextdomain(PACKAGE, LOCALEDIR);
-	textdomain(PACKAGE);
-	atexit(close_stdout);
-
-	if (argc < 2)
-		usage(stderr);
-
-	/* Parse arguments. */
-	parse_option(&ctl, argc, argv);
-
-	/* Find out terminal name. */
-	if (!ctl.opt_te_terminal_name) {
-		ctl.opt_te_terminal_name = getenv("TERM");
-		if (ctl.opt_te_terminal_name == NULL)
+	if (!ctl->opt_te_terminal_name) {
+		ctl->opt_te_terminal_name = getenv("TERM");
+		if (ctl->opt_te_terminal_name == NULL)
 			errx(EXIT_FAILURE, _("$TERM is not defined."));
 	}
 
 	/* Find terminfo entry. */
-	if (setupterm(ctl.opt_te_terminal_name, 1, &term_errno))
-		switch(term_errno) {
+	if (setupterm(ctl->opt_te_terminal_name, STDOUT_FILENO, &term_errno))
+		switch (term_errno) {
 		case -1:
 			errx(EXIT_FAILURE, _("terminfo database cannot be found"));
 		case 0:
-			errx(EXIT_FAILURE, _("%s: unknown terminal type"), ctl.opt_te_terminal_name);
+			errx(EXIT_FAILURE, _("%s: unknown terminal type"), ctl->opt_te_terminal_name);
 		case 1:
 			errx(EXIT_FAILURE, _("terminal is hardcopy"));
 		}
 
 	/* See if the terminal is a virtual console terminal. */
-	ctl.vcterm = (!strncmp(ctl.opt_te_terminal_name, "con", 3) ||
-		      !strncmp(ctl.opt_te_terminal_name, "linux", 5));
+	ctl->vcterm = (!strncmp(ctl->opt_te_terminal_name, "con", 3) ||
+		       !strncmp(ctl->opt_te_terminal_name, "linux", 5));
+}
+
 
-	/* Perform the selected options. */
+int main(int argc, char **argv)
+{
+	struct setterm_control ctl;
+
+	memset(&ctl, 0, sizeof (ctl));
+	setlocale(LC_ALL, "");
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	if (argc < 2)
+		usage(stderr);
+
+	parse_option(&ctl, argc, argv);
+	init_terminal(&ctl);
 	perform_sequence(&ctl);
 
 	return EXIT_SUCCESS;
-- 
1.9.2

[PATCH 08/17] setterm: clean up screendump()

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 92 +++++++++++++++++++++++++++-------------------------
 1 file changed, 47 insertions(+), 45 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 05ea387..d889db4 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -144,7 +144,7 @@ enum {
 #define	TABS_MAX	160
 #define BLENGTH_MAX	2000
 
-/* Option controls. */
+/* Command controls. */
 struct setterm_control {
 	char *opt_te_terminal_name;	/* terminal name */
 	int opt_bl_min;		/* blank screen */
@@ -152,6 +152,7 @@ struct setterm_control {
 	int opt_bfreq_f;	/* bell frequency in Hz */
 	int opt_sn_num;		/* console number to be snapshoted */
 	char *opt_sn_name;	/* path to write snap */
+	char *in_device;	/* device to snapshot */
 	int opt_msglevel_num;	/* printk() loging level */
 	int opt_ps_mode;	/* powersave mode */
 	int opt_pd_min;		/* powerdown time */
@@ -721,15 +722,18 @@ static void show_tabs(void)
 	}
 }
 
-static void screendump(struct setterm_control *ctl, FILE *F)
+static void __attribute__((__noreturn__)) read_error(struct setterm_control *ctl)
+{
+	if (ctl->opt_sn_num != 0)
+		errx(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
+	else
+		errx(EXIT_DUMPFILE, _("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
+}
+
+static int open_snapshot_device(struct setterm_control *ctl)
 {
 	char infile[MAXPATHLEN];
-	unsigned char header[4];
-	unsigned int rows, cols;
 	int fd;
-	size_t i, j;
-	ssize_t rc;
-	char *inbuf = NULL, *outbuf = NULL, *p, *q;
 
 	sprintf(infile, "/dev/vcsa%d", ctl->opt_sn_num);
 	fd = open(infile, O_RDONLY);
@@ -744,25 +748,47 @@ static void screendump(struct setterm_control *ctl, FILE *F)
 		sprintf(infile, "/dev/vcc/a%.0u", ctl->opt_sn_num);
 		fd = open(infile, O_RDONLY);
 	}
-	if (fd < 0) {
-		sprintf(infile, "/dev/vcsa%d", ctl->opt_sn_num);
-		goto read_error;
-	}
+	ctl->in_device = infile;
+	if (fd < 0)
+		read_error(ctl);
+	return fd;
+}
+
+static void screendump(struct setterm_control *ctl)
+{
+	unsigned char header[4];
+	unsigned int rows, cols;
+	int fd;
+	FILE *out;
+	size_t i, j;
+	ssize_t rc;
+	char *inbuf, *outbuf, *p, *q;
+	char dump_default[] = "screen.dump";
+
+	/* open source and destination files */
+	fd = open_snapshot_device(ctl);
+	if (!ctl->opt_sn_name)
+		ctl->opt_sn_name = dump_default;
+	out = fopen(ctl->opt_sn_name, ctl->opt_snap ? "w" : "a");
+	if (!out)
+		err(EXIT_DUMPFILE, _("can not open dump file %s for output"), ctl->opt_sn_name);
+	/* determine snapshot size */
 	if (read(fd, header, 4) != 4)
-		goto read_error;
+		read_error(ctl);
 	rows = header[0];
 	cols = header[1];
 	if (rows * cols == 0)
-		goto read_error;
-
+		read_error(ctl);
+	/* allocate buffers */
 	inbuf = xmalloc(rows * cols * 2);
 	outbuf = xmalloc(rows * (cols + 1));
-
+	/* read input */
 	rc = read(fd, inbuf, rows * cols * 2);
 	if (rc < 0 || (size_t)rc != rows * cols * 2)
-		goto read_error;
+		read_error(ctl);
 	p = inbuf;
 	q = outbuf;
+	/* copy inbuf to outbuf */
 	for (i = 0; i < rows; i++) {
 		for (j = 0; j < cols; j++) {
 			*q++ = *p;
@@ -772,27 +798,14 @@ static void screendump(struct setterm_control *ctl, FILE *F)
 			q--;
 		*q++ = '\n';
 	}
-	if (fwrite(outbuf, 1, q - outbuf, F) != (size_t)(q - outbuf)) {
-		warnx(_("Error writing screendump"));
-		goto error;
-	}
+	fwrite(outbuf, 1, q - outbuf, out);
+	/* clean up allocations */
 	close(fd);
 	free(inbuf);
 	free(outbuf);
+	if (close_stream(out) != 0)
+		errx(EXIT_FAILURE, _("write error"));
 	return;
-
- read_error:
-	if (ctl->opt_sn_num != 0)
-		warnx(_("Couldn't read %s"), infile);
-	else
-		warnx(_("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
-
- error:
-	if (fd >= 0)
-		close(fd);
-	free(inbuf);
-	free(outbuf);
-	exit(EXIT_FAILURE);
 }
 
 static void perform_sequence(struct setterm_control *ctl)
@@ -1013,18 +1026,7 @@ static void perform_sequence(struct setterm_control *ctl)
 
 	/* -snap [1-NR_CONS]. */
 	if (ctl->opt_snap || ctl->opt_append) {
-		FILE *F;
-		char dump_default[] = "screen.dump";
-
-		if (!ctl->opt_sn_name)
-			ctl->opt_sn_name = dump_default;
-		F = fopen(ctl->opt_sn_name, ctl->opt_snap ? "w" : "a");
-		if (!F)
-			err(EXIT_DUMPFILE, _("can not open dump file %s for output"),
-				ctl->opt_sn_name);
-		screendump(ctl, F);
-		if (close_stream(F) != 0)
-			errx(EXIT_FAILURE, _("write error"));
+		screendump(ctl);
 	}
 
 	/* -msg [on|off]. */
-- 
1.9.2

[PATCH 09/17] setterm: remove devfs and /dev/vcsa0 support

[Sami Kerola]

The devfs files /dev/vcc/a* does not need to be supported, and vcsa0 has
not existed in years if ever.

Reference: http://lwn.net/Articles/65197/
Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devices.txt?id=14186fea0cb06bc43181ce239efe0df6f1af260a#n260
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 bash-completion/setterm |  2 +-
 term-utils/setterm.c    | 26 +++++++-------------------
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/bash-completion/setterm b/bash-completion/setterm
index a7ef6eb..f47debf 100644
--- a/bash-completion/setterm
+++ b/bash-completion/setterm
@@ -40,7 +40,7 @@ _setterm_module()
 			;;
 		'-dump'|'-append')
 			local NUM_CONS
-			NUM_CONS=(/sys/class/tty/*)
+			NUM_CONS=(/dev/vcsa?*)
 			COMPREPLY=( $(compgen -W "{1..${#NUM_CONS[*]}}" -- $cur) )
 			return 0
 			;;
diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index d889db4..3a02347 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -724,31 +724,18 @@ static void show_tabs(void)
 
 static void __attribute__((__noreturn__)) read_error(struct setterm_control *ctl)
 {
-	if (ctl->opt_sn_num != 0)
-		errx(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
-	else
-		errx(EXIT_DUMPFILE, _("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
+	err(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
 }
 
 static int open_snapshot_device(struct setterm_control *ctl)
 {
-	char infile[MAXPATHLEN];
 	int fd;
 
-	sprintf(infile, "/dev/vcsa%d", ctl->opt_sn_num);
-	fd = open(infile, O_RDONLY);
-	if (fd < 0 && ctl->opt_sn_num == 0) {
-		/* vcsa0 is often called vcsa */
-		sprintf(infile, "/dev/vcsa");
-		fd = open(infile, O_RDONLY);
-	}
-	if (fd < 0) {
-		/* try devfs name - for zero ctl->opt_sn_num just /dev/vcc/a */
-		/* some gcc's warn for %.u - add 0 */
-		sprintf(infile, "/dev/vcc/a%.0u", ctl->opt_sn_num);
-		fd = open(infile, O_RDONLY);
-	}
-	ctl->in_device = infile;
+	if (ctl->opt_sn_num)
+		xasprintf(&ctl->in_device, "/dev/vcsa%d", ctl->opt_sn_num);
+	else
+		xasprintf(&ctl->in_device, "/dev/vcsa");
+	fd = open(ctl->in_device, O_RDONLY);
 	if (fd < 0)
 		read_error(ctl);
 	return fd;
@@ -803,6 +790,7 @@ static void screendump(struct setterm_control *ctl)
 	close(fd);
 	free(inbuf);
 	free(outbuf);
+	free(ctl->in_device);
 	if (close_stream(out) != 0)
 		errx(EXIT_FAILURE, _("write error"));
 	return;
-- 
1.9.2

[PATCH 10/17] setterm: make -msglevel 0 to work as is did earlier

[Sami Kerola]

Commit 3393c136 caused regression.  The klogctl() logging range is 1-8,
but the value 0 is special according to setterm.1 manual page.  It turns
on loging, same way as '-msg on' option.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 bash-completion/setterm |  2 +-
 term-utils/setterm.c    | 12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/bash-completion/setterm b/bash-completion/setterm
index f47debf..8e0f629 100644
--- a/bash-completion/setterm
+++ b/bash-completion/setterm
@@ -51,7 +51,7 @@ _setterm_module()
 			return 0
 			;;
 		'-msglevel')
-			COMPREPLY=( $(compgen -W "{1..8}" -- $cur) )
+			COMPREPLY=( $(compgen -W "{0..8}" -- $cur) )
 			return 0
 			;;
 		'-powersave')
diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 3a02347..2f9151c 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -134,7 +134,7 @@ enum {
 
 /* Console log levels */
 enum {
-	CONSOLE_LEVEL_MIN = 1,
+	CONSOLE_LEVEL_MIN = 0,
 	CONSOLE_LEVEL_MAX = 8
 };
 
@@ -446,16 +446,14 @@ usage(FILE *out) {
 	fputs(_(" -append <1-NR_CONSOLES>\n"), out);
 	fputs(_(" -file dumpfilename\n"), out);
 	fputs(_(" -msg <on|off>\n"), out);
-	fputs(_(" -msglevel <0-8>\n"), out);	/* FIXME: klogctl console_log range is 1-8 */
+	fputs(_(" -msglevel <0-8>\n"), out);
 	fputs(_(" -powersave <on|vsync|hsync|powerdown|off>\n"), out);
 	fputs(_(" -powerdown <0-60>\n"), out);
 	fputs(_(" -blength <0-2000>\n"), out);
 	fputs(_(" -bfreq freqnumber\n"), out);
 	fputs(_(" -version\n"), out);
 	fputs(_(" -help\n"), out);
-
 	fprintf(out, USAGE_MAN_TAIL("setterm(1)"));
-
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
@@ -660,6 +658,10 @@ static void parse_option(struct setterm_control *ctl, int argc, char **argv)
 		case OPT_MSGLEVEL:
 			ctl->opt_msglevel = set_opt_flag(ctl->opt_msglevel);
 			ctl->opt_msglevel_num = parse_msglevel(optarg);
+			if (ctl->opt_msglevel_num == 0) {
+				ctl->opt_msg = set_opt_flag(ctl->opt_msg);
+				ctl->opt_msg_on |= 1;
+			}
 			break;
 		case OPT_POWERSAVE:
 			ctl->opt_powersave = set_opt_flag(ctl->opt_powersave);
@@ -1031,7 +1033,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -msglevel [0-8] */
-	if (ctl->opt_msglevel && ctl->vcterm) {
+	if (ctl->opt_msglevel_num && ctl->vcterm) {
 		/* 8 -- Set level of messages printed to console */
 		result = klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL, ctl->opt_msglevel_num);
 		if (result != 0)
-- 
1.9.2

[PATCH 11/17] setterm: correct usage() bright color argument

[Sami Kerola]

This has been wrong since commit 2dc8716.  The bright is a prefix
attribute to other colors.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 bash-completion/setterm | 15 ++++++++++++---
 term-utils/setterm.c    | 21 +++++++++------------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/bash-completion/setterm b/bash-completion/setterm
index 8e0f629..69f6620 100644
--- a/bash-completion/setterm
+++ b/bash-completion/setterm
@@ -1,6 +1,6 @@
 _setterm_module()
 {
-	local cur prev OPTS
+	local bright cur prev OPTS
 	COMPREPLY=()
 	cur="${COMP_WORDS[COMP_CWORD]}"
 	prev="${COMP_WORDS[COMP_CWORD-1]}"
@@ -14,8 +14,17 @@ _setterm_module()
 			COMPREPLY=( $(compgen -W "$TERM_LIST" -- $cur) )
 			return 0
 			;;
-		'-foreground'|'-background'|'-ulcolor'|'-hbcolor')
-			COMPREPLY=( $(compgen -W "default black blue cyan green magenta red white yellow" -- $cur) )
+		'-foreground'|'-background')
+			COMPREPLY=( $(compgen -W "black blue cyan default green magenta red white yellow" -- $cur) )
+			return 0
+			;;
+		'-ulcolor'|'-hbcolor'|'bright')
+			if [ $prev != 'bright' ]; then
+				bright='bright black grey'
+			else
+				bright=''
+			fi
+			COMPREPLY=( $(compgen -W "$bright blue cyan green magenta red white yellow" -- $cur) )
 			return 0
 			;;
 		'-cursor'|'-repeat'|'-appcursorkeys'|'-linewrap'|'-inversescreen'|'-bold'|'-half-bright'|'-blink'|'-reverse'|'-underline'|'-msg')
diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 2f9151c..3b4ab56 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -404,18 +404,15 @@ static int parse_bfreq(char **argv, char *optarg, int *optind)
 	return strtoint_or_err(arg);
 }
 
-static void __attribute__ ((__noreturn__))
-usage(FILE *out) {
-/* Print error message about arguments, and the command's syntax. */
-
+static void __attribute__((__noreturn__)) usage(FILE *out)
+{
 	if (out == stderr)
-		warnx(_("Argument error."));
+		warnx(_("argument error"));
 
-	fputs(_("\nUsage:\n"), out);
+	fputs(USAGE_HEADER, out);
 	fprintf(out,
 	      _(" %s [options]\n"), program_invocation_short_name);
-
-	fputs(_("\nOptions:\n"), out);
+	fputs(USAGE_OPTIONS, out);
 	fputs(_(" -term <terminal_name>\n"), out);
 	fputs(_(" -reset\n"), out);
 	fputs(_(" -initialize\n"), out);
@@ -426,10 +423,10 @@ usage(FILE *out) {
 	fputs(_(" -default\n"), out);
 	fputs(_(" -foreground <default|black|blue|cyan|green|magenta|red|white|yellow>\n"), out);
 	fputs(_(" -background <default|black|blue|cyan|green|magenta|red|white|yellow>\n"), out);
-	fputs(_(" -ulcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
-	fputs(_(" -ulcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
-	fputs(_(" -hbcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
-	fputs(_(" -hbcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
+	fputs(_(" -ulcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
+	fputs(_(" -ulcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
+	fputs(_(" -hbcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
+	fputs(_(" -hbcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
 	fputs(_(" -inversescreen <on|off>\n"), out);
 	fputs(_(" -bold <on|off>\n"), out);
 	fputs(_(" -half-bright <on|off>\n"), out);
-- 
1.9.2

[PATCH 12/17] setterm: improve perform_sequence() coding style

[Sami Kerola]

Delete pointless braces, new lines, and add spacing between operators.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 92 ++++++++++++++++++++--------------------------------
 1 file changed, 35 insertions(+), 57 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 3b4ab56..f1ed862 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -689,13 +689,10 @@ static void parse_option(struct setterm_control *ctl, int argc, char **argv)
 
 /* End of command line parsing routines. */
 
-static char *ti_entry(const char *name) {
-	/* name: Terminfo capability string to lookup. */
-
-/* Return the specified terminfo string, or an empty string if no such terminfo
- * capability exists.
- */
-
+/* Return the specified terminfo string, or an empty string if no such
+ * terminfo capability exists.  */
+static char *ti_entry(const char *name)
+{
 	char *buf_ptr;
 
 	if ((buf_ptr = tigetstr((char *)name)) == (char *)-1)
@@ -798,17 +795,14 @@ static void screendump(struct setterm_control *ctl)
 static void perform_sequence(struct setterm_control *ctl)
 {
 	int result;
-/* Perform the selected options. */
 
 	/* -reset. */
-	if (ctl->opt_reset) {
+	if (ctl->opt_reset)
 		putp(ti_entry("rs1"));
-	}
 
 	/* -initialize. */
-	if (ctl->opt_initialize) {
+	if (ctl->opt_initialize)
 		putp(ti_entry("is2"));
-	}
 
 	/* -cursor [on|off]. */
 	if (ctl->opt_cursor) {
@@ -831,8 +825,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		fputs(ctl->opt_appck_on ? "\033[?1h" : "\033[?1l", stdout);
 
 	/* -default.  Vc sets default rendition, otherwise clears all
-	 * attributes.
-	 */
+	 * attributes. */
 	if (ctl->opt_default) {
 		if (ctl->vcterm)
 			printf("\033[0m");
@@ -841,39 +834,31 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -foreground black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only (ANSI).
-	 */
+	 * Vc only (ANSI). */
 	if (ctl->opt_foreground && ctl->vcterm)
 		printf("\033[3%c%s", '0' + ctl->opt_fo_color, "m");
 
 	/* -background black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only (ANSI).
-	 */
+	 * Vc only (ANSI). */
 	if (ctl->opt_background && ctl->vcterm)
 		printf("\033[4%c%s", '0' + ctl->opt_ba_color, "m");
 
 	/* -ulcolor black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only.
-	 */
-	if (ctl->opt_ulcolor && ctl->vcterm) {
+	 * Vc only. */
+	if (ctl->opt_ulcolor && ctl->vcterm)
 		printf("\033[1;%d]", ctl->opt_ul_color);
-	}
 
 	/* -hbcolor black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only.
-	 */
-	if (ctl->opt_hbcolor && ctl->vcterm) {
+	 * Vc only. */
+	if (ctl->opt_hbcolor && ctl->vcterm)
 		printf("\033[2;%d]", ctl->opt_hb_color);
-	}
 
-	/* -inversescreen [on|off].  Vc only (vt102).
-	 */
+	/* -inversescreen [on|off].  Vc only (vt102). */
 	if (ctl->opt_inversescreen && ctl->vcterm)
 		fputs(ctl->opt_invsc_on ? "\033[?5h" : "\033[?5l", stdout);
 
 	/* -bold [on|off].  Vc behaves as expected, otherwise off turns off
-	 * all attributes.
-	 */
+	 * all attributes. */
 	if (ctl->opt_bold) {
 		if (ctl->opt_bo_on)
 			putp(ti_entry("bold"));
@@ -885,9 +870,8 @@ static void perform_sequence(struct setterm_control *ctl)
 		}
 	}
 
-	/* -half-bright [on|off].  Vc behaves as expected, otherwise off turns off
-	 * all attributes.
-	 */
+	/* -half-bright [on|off].  Vc behaves as expected, otherwise off
+	 * turns off all attributes.  */
 	if (ctl->opt_halfbright) {
 		if (ctl->opt_hb_on)
 			putp(ti_entry("dim"));
@@ -900,8 +884,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -blink [on|off].  Vc behaves as expected, otherwise off turns off
-	 * all attributes.
-	 */
+	 * all attributes. */
 	if (ctl->opt_blink) {
 		if (ctl->opt_bl_on)
 			putp(ti_entry("blink"));
@@ -914,8 +897,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -reverse [on|off].  Vc behaves as expected, otherwise off turns
-	 * off all attributes.
-	 */
+	 * off all attributes. */
 	if (ctl->opt_reverse) {
 		if (ctl->opt_re_on)
 			putp(ti_entry("rev"));
@@ -946,7 +928,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_tb_array[0] == -1)
 			show_tabs();
 		else {
-			for(i=0; ctl->opt_tb_array[i] > 0; i++)
+			for (i = 0; ctl->opt_tb_array[i] > 0; i++)
 				printf("\033[%dG\033H", ctl->opt_tb_array[i]);
 			putchar('\r');
 		}
@@ -959,7 +941,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_tb_array[0] == -1)
 			fputs("\033[3g", stdout);
 		else
-			for(i=0; ctl->opt_tb_array[i] > 0; i++)
+			for (i = 0; ctl->opt_tb_array[i] > 0; i++)
 				printf("\033[%dG\033[g", ctl->opt_tb_array[i]);
 		putchar('\r');
 	}
@@ -969,8 +951,8 @@ static void perform_sequence(struct setterm_control *ctl)
 		int i;
 
 		fputs("\033[3g\r", stdout);
-		for(i=ctl->opt_rt_len+1; i<=TABS_MAX; i+=ctl->opt_rt_len)
-			printf("\033[%dC\033H",ctl->opt_rt_len);
+		for (i = ctl->opt_rt_len + 1; i <= TABS_MAX; i += ctl->opt_rt_len)
+			printf("\033[%dC\033H", ctl->opt_rt_len);
 		putchar('\r');
 	}
 
@@ -980,20 +962,20 @@ static void perform_sequence(struct setterm_control *ctl)
 			printf("\033[9;%d]", ctl->opt_bl_min);
 		else if (ctl->opt_bl_min == BLANKSCREEN) {
 			char ioctlarg = TIOCL_BLANKSCREEN;
-			if (ioctl(0,TIOCLINUX,&ioctlarg))
+			if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
 				warn(_("cannot force blank"));
 		} else if (ctl->opt_bl_min == UNBLANKSCREEN) {
 			char ioctlarg = TIOCL_UNBLANKSCREEN;
-			if (ioctl(0,TIOCLINUX,&ioctlarg))
+			if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
 				warn(_("cannot force unblank"));
 		} else if (ctl->opt_bl_min == BLANKEDSCREEN) {
 			char ioctlarg = TIOCL_BLANKEDSCREEN;
 			int ret;
-			ret = ioctl(0,TIOCLINUX,&ioctlarg);
+			ret = ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg);
 			if (ret < 0)
 				warn(_("cannot get blank status"));
 			else
-				printf("%d\n",ret);
+				printf("%d\n", ret);
 		}
 	}
 
@@ -1002,37 +984,34 @@ static void perform_sequence(struct setterm_control *ctl)
 		char ioctlarg[2];
 		ioctlarg[0] = TIOCL_SETVESABLANK;
 		ioctlarg[1] = ctl->opt_ps_mode;
-		if (ioctl(0,TIOCLINUX,ioctlarg))
+		if (ioctl(STDIN_FILENO, TIOCLINUX, ioctlarg))
 			warn(_("cannot (un)set powersave mode"));
 	}
 
 	/* -powerdown [0-60]. */
-	if (ctl->opt_powerdown) {
+	if (ctl->opt_powerdown)
 		printf("\033[14;%d]", ctl->opt_pd_min);
-	}
 
 	/* -snap [1-NR_CONS]. */
-	if (ctl->opt_snap || ctl->opt_append) {
+	if (ctl->opt_snap || ctl->opt_append)
 		screendump(ctl);
-	}
 
-	/* -msg [on|off]. */
+	/* -msg [on|off].  Controls printk's to console. */
 	if (ctl->opt_msg && ctl->vcterm) {
 		if (ctl->opt_msg_on)
-			/* 7 -- Enable printk's to console */
 			result = klogctl(SYSLOG_ACTION_CONSOLE_ON, NULL, 0);
 		else
-			/*  6 -- Disable printk's to console */
 			result = klogctl(SYSLOG_ACTION_CONSOLE_OFF, NULL, 0);
 
 		if (result != 0)
 			warn(_("klogctl error"));
 	}
 
-	/* -msglevel [0-8] */
+	/* -msglevel [0-8].  Console printk message level. */
 	if (ctl->opt_msglevel_num && ctl->vcterm) {
-		/* 8 -- Set level of messages printed to console */
-		result = klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL, ctl->opt_msglevel_num);
+		result =
+		    klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL,
+			    ctl->opt_msglevel_num);
 		if (result != 0)
 			warn(_("klogctl error"));
 	}
@@ -1046,7 +1025,6 @@ static void perform_sequence(struct setterm_control *ctl)
 	if (ctl->opt_bfreq && ctl->vcterm) {
 		printf("\033[10;%d]", ctl->opt_bfreq_f);
 	}
-
 }
 
 static void init_terminal(struct setterm_control *ctl)
-- 
1.9.2

[PATCH 13/17] setterm: tell user when options does not effect

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 83 ++++++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index f1ed862..2a0162e 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -792,6 +792,17 @@ static void screendump(struct setterm_control *ctl)
 	return;
 }
 
+/* Some options are applicable when terminal is virtual console. */
+static int vc_only(struct setterm_control *ctl, const char *err)
+{
+	if (!ctl->vcterm) {
+		if (err)
+			warnx(_("terminal %s does not support %s"),
+			      ctl->opt_te_terminal_name, err);
+	}
+	return ctl->vcterm;
+}
+
 static void perform_sequence(struct setterm_control *ctl)
 {
 	int result;
@@ -812,49 +823,45 @@ static void perform_sequence(struct setterm_control *ctl)
 			putp(ti_entry("civis"));
 	}
 
-	/* -linewrap [on|off]. Vc only (vt102) */
-	if (ctl->opt_linewrap && ctl->vcterm)
+	/* -linewrap [on|off]. */
+	if (ctl->opt_linewrap && vc_only(ctl, "-linewrap"))
 		fputs(ctl->opt_li_on ? "\033[?7h" : "\033[?7l", stdout);
 
-	/* -repeat [on|off]. Vc only (vt102) */
-	if (ctl->opt_repeat && ctl->vcterm)
+	/* -repeat [on|off]. */
+	if (ctl->opt_repeat && vc_only(ctl, "-repeat"))
 		fputs(ctl->opt_rep_on ? "\033[?8h" : "\033[?8l", stdout);
 
-	/* -appcursorkeys [on|off]. Vc only (vt102) */
-	if (ctl->opt_appcursorkeys && ctl->vcterm)
+	/* -appcursorkeys [on|off]. */
+	if (ctl->opt_appcursorkeys && vc_only(ctl, "-appcursorkeys"))
 		fputs(ctl->opt_appck_on ? "\033[?1h" : "\033[?1l", stdout);
 
 	/* -default.  Vc sets default rendition, otherwise clears all
 	 * attributes. */
 	if (ctl->opt_default) {
-		if (ctl->vcterm)
+		if (vc_only(ctl, NULL))
 			printf("\033[0m");
 		else
 			putp(ti_entry("sgr0"));
 	}
 
-	/* -foreground black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only (ANSI). */
-	if (ctl->opt_foreground && ctl->vcterm)
+	/* -foreground black|red|green|yellow|blue|magenta|cyan|white|default. */
+	if (ctl->opt_foreground && vc_only(ctl, "-foreground"))
 		printf("\033[3%c%s", '0' + ctl->opt_fo_color, "m");
 
-	/* -background black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only (ANSI). */
-	if (ctl->opt_background && ctl->vcterm)
+	/* -background black|red|green|yellow|blue|magenta|cyan|white|default. */
+	if (ctl->opt_background && vc_only(ctl, "-background"))
 		printf("\033[4%c%s", '0' + ctl->opt_ba_color, "m");
 
-	/* -ulcolor black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only. */
-	if (ctl->opt_ulcolor && ctl->vcterm)
+	/* -ulcolor black|red|green|yellow|blue|magenta|cyan|white|default. */
+	if (ctl->opt_ulcolor && vc_only(ctl, "-ulcolor"))
 		printf("\033[1;%d]", ctl->opt_ul_color);
 
-	/* -hbcolor black|red|green|yellow|blue|magenta|cyan|white|default.
-	 * Vc only. */
-	if (ctl->opt_hbcolor && ctl->vcterm)
+	/* -hbcolor black|red|green|yellow|blue|magenta|cyan|white|default. */
+	if (ctl->opt_hbcolor && vc_only(ctl, "-hbcolor"))
 		printf("\033[2;%d]", ctl->opt_hb_color);
 
-	/* -inversescreen [on|off].  Vc only (vt102). */
-	if (ctl->opt_inversescreen && ctl->vcterm)
+	/* -inversescreen [on|off]. */
+	if (ctl->opt_inversescreen && vc_only(ctl, "-inversescreen"))
 		fputs(ctl->opt_invsc_on ? "\033[?5h" : "\033[?5l", stdout);
 
 	/* -bold [on|off].  Vc behaves as expected, otherwise off turns off
@@ -863,7 +870,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_bo_on)
 			putp(ti_entry("bold"));
 		else {
-			if (ctl->vcterm)
+			if (vc_only(ctl, NULL))
 				fputs("\033[22m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -876,7 +883,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_hb_on)
 			putp(ti_entry("dim"));
 		else {
-			if (ctl->vcterm)
+			if (vc_only(ctl, NULL))
 				fputs("\033[22m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -889,7 +896,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_bl_on)
 			putp(ti_entry("blink"));
 		else {
-			if (ctl->vcterm)
+			if (vc_only(ctl, NULL))
 				fputs("\033[25m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -902,7 +909,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		if (ctl->opt_re_on)
 			putp(ti_entry("rev"));
 		else {
-			if (ctl->vcterm)
+			if (vc_only(ctl, NULL))
 				fputs("\033[27m", stdout);
 			else
 				putp(ti_entry("sgr0"));
@@ -913,16 +920,16 @@ static void perform_sequence(struct setterm_control *ctl)
 	if (ctl->opt_underline)
 		putp(ti_entry(ctl->opt_un_on ? "smul" : "rmul"));
 
-	/* -store.  Vc only. */
-	if (ctl->opt_store && ctl->vcterm)
+	/* -store. */
+	if (ctl->opt_store && vc_only(ctl, "-store"))
 		fputs("\033[8]", stdout);
 
 	/* -clear [all|rest]. */
 	if (ctl->opt_clear)
 		putp(ti_entry(ctl->opt_cl_all ? "clear" : "ed"));
 
-	/* -tabs Vc only. */
-	if (ctl->opt_tabs && ctl->vcterm) {
+	/* -tabs. */
+	if (ctl->opt_tabs && vc_only(ctl, "-tabs")) {
 		int i;
 
 		if (ctl->opt_tb_array[0] == -1)
@@ -934,8 +941,8 @@ static void perform_sequence(struct setterm_control *ctl)
 		}
 	}
 
-	/* -clrtabs Vc only. */
-	if (ctl->opt_clrtabs && ctl->vcterm) {
+	/* -clrtabs. */
+	if (ctl->opt_clrtabs && vc_only(ctl, "-clrtabs")) {
 		int i;
 
 		if (ctl->opt_tb_array[0] == -1)
@@ -946,8 +953,8 @@ static void perform_sequence(struct setterm_control *ctl)
 		putchar('\r');
 	}
 
-	/* -regtabs Vc only. */
-	if (ctl->opt_regtabs && ctl->vcterm) {
+	/* -regtabs. */
+	if (ctl->opt_regtabs && vc_only(ctl, "-regtabs")) {
 		int i;
 
 		fputs("\033[3g\r", stdout);
@@ -957,7 +964,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -blank [0-60]. */
-	if (ctl->opt_blank && ctl->vcterm) {
+	if (ctl->opt_blank && vc_only(ctl, "-blank")) {
 		if (ctl->opt_bl_min >= 0)
 			printf("\033[9;%d]", ctl->opt_bl_min);
 		else if (ctl->opt_bl_min == BLANKSCREEN) {
@@ -997,7 +1004,7 @@ static void perform_sequence(struct setterm_control *ctl)
 		screendump(ctl);
 
 	/* -msg [on|off].  Controls printk's to console. */
-	if (ctl->opt_msg && ctl->vcterm) {
+	if (ctl->opt_msg && vc_only(ctl, "-msg")) {
 		if (ctl->opt_msg_on)
 			result = klogctl(SYSLOG_ACTION_CONSOLE_ON, NULL, 0);
 		else
@@ -1008,7 +1015,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -msglevel [0-8].  Console printk message level. */
-	if (ctl->opt_msglevel_num && ctl->vcterm) {
+	if (ctl->opt_msglevel_num && vc_only(ctl, "-msglevel")) {
 		result =
 		    klogctl(SYSLOG_ACTION_CONSOLE_LEVEL, NULL,
 			    ctl->opt_msglevel_num);
@@ -1017,12 +1024,12 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -blength [0-2000] */
-	if (ctl->opt_blength && ctl->vcterm) {
+	if (ctl->opt_blength && vc_only(ctl, "-blength")) {
 		printf("\033[11;%d]", ctl->opt_blength_l);
 	}
 
 	/* -bfreq freqnumber */
-	if (ctl->opt_bfreq && ctl->vcterm) {
+	if (ctl->opt_bfreq && vc_only(ctl, "-bfreq")) {
 		printf("\033[10;%d]", ctl->opt_bfreq_f);
 	}
 }
-- 
1.9.2

[PATCH 14/17] setterm: improve error messages

[Sami Kerola]

Reuse messages to make translation work easier.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 2a0162e..2b1901f 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -262,7 +262,7 @@ static int parse_ulhb_color(char **argv, int *optind)
 	else {
 		color = strtoint_or_err(color_name);
 		if (color < BLACK || DEFAULT < color)
-			errx(EXIT_FAILURE, _("argument error"));
+			errx(EXIT_FAILURE, _("argument error: %s"), color_name);
 	}
 	if (bright && (color == BLACK || color == GREY))
 		errx(EXIT_FAILURE, _("argument error: bright %s is not supported"), color_name);
@@ -720,7 +720,7 @@ static void show_tabs(void)
 
 static void __attribute__((__noreturn__)) read_error(struct setterm_control *ctl)
 {
-	err(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
+	err(EXIT_DUMPFILE, _("cannot read %s"), ctl->in_device);
 }
 
 static int open_snapshot_device(struct setterm_control *ctl)
-- 
1.9.2

[PATCH 15/17] setterm: various visual terminal effects are not console specific

[Sami Kerola]

Setting colors, making line not to wrap, and so on can be attempted in
other than consoles.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 2b1901f..10ca021 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -824,7 +824,7 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -linewrap [on|off]. */
-	if (ctl->opt_linewrap && vc_only(ctl, "-linewrap"))
+	if (ctl->opt_linewrap)
 		fputs(ctl->opt_li_on ? "\033[?7h" : "\033[?7l", stdout);
 
 	/* -repeat [on|off]. */
@@ -845,11 +845,11 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -foreground black|red|green|yellow|blue|magenta|cyan|white|default. */
-	if (ctl->opt_foreground && vc_only(ctl, "-foreground"))
+	if (ctl->opt_foreground)
 		printf("\033[3%c%s", '0' + ctl->opt_fo_color, "m");
 
 	/* -background black|red|green|yellow|blue|magenta|cyan|white|default. */
-	if (ctl->opt_background && vc_only(ctl, "-background"))
+	if (ctl->opt_background)
 		printf("\033[4%c%s", '0' + ctl->opt_ba_color, "m");
 
 	/* -ulcolor black|red|green|yellow|blue|magenta|cyan|white|default. */
@@ -857,11 +857,11 @@ static void perform_sequence(struct setterm_control *ctl)
 		printf("\033[1;%d]", ctl->opt_ul_color);
 
 	/* -hbcolor black|red|green|yellow|blue|magenta|cyan|white|default. */
-	if (ctl->opt_hbcolor && vc_only(ctl, "-hbcolor"))
+	if (ctl->opt_hbcolor)
 		printf("\033[2;%d]", ctl->opt_hb_color);
 
 	/* -inversescreen [on|off]. */
-	if (ctl->opt_inversescreen && vc_only(ctl, "-inversescreen"))
+	if (ctl->opt_inversescreen)
 		fputs(ctl->opt_invsc_on ? "\033[?5h" : "\033[?5l", stdout);
 
 	/* -bold [on|off].  Vc behaves as expected, otherwise off turns off
@@ -929,12 +929,12 @@ static void perform_sequence(struct setterm_control *ctl)
 		putp(ti_entry(ctl->opt_cl_all ? "clear" : "ed"));
 
 	/* -tabs. */
-	if (ctl->opt_tabs && vc_only(ctl, "-tabs")) {
-		int i;
-
+	if (ctl->opt_tabs) {
 		if (ctl->opt_tb_array[0] == -1)
 			show_tabs();
 		else {
+			int i;
+
 			for (i = 0; ctl->opt_tb_array[i] > 0; i++)
 				printf("\033[%dG\033H", ctl->opt_tb_array[i]);
 			putchar('\r');
-- 
1.9.2

[PATCH 16/17] setterm: mark some options to be exclusive with each other

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 10ca021..ba175f9 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -78,6 +78,7 @@
 #include "c.h"
 #include "closestream.h"
 #include "nls.h"
+#include "optutils.h"
 #include "strutils.h"
 #include "xalloc.h"
 
@@ -541,8 +542,16 @@ static void parse_option(struct setterm_control *ctl, int argc, char **argv)
 		{"help", no_argument, NULL, OPT_HELP},
 		{NULL, 0, NULL, 0}
 	};
+	static const ul_excl_t excl[] = {
+		{ OPT_DEFAULT, OPT_STORE },
+		{ OPT_TABS, OPT_CLRTABS, OPT_REGTABS },
+		{ OPT_MSG, OPT_MSGLEVEL },
+		{ 0 }
+	};
+	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
 
 	while ((c = getopt_long_only(argc, argv, "", longopts, NULL)) != -1) {
+		err_exclusive_options(c, longopts, excl, excl_st);
 		switch (c) {
 		case OPT_TERM:
 			ctl->opt_term = set_opt_flag(ctl->opt_term);
-- 
1.9.2

[PATCH 17/17] setterm: add set_blanking() action

[Sami Kerola]

This make over long perform_sequence() function a little bit shorter.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/setterm.c | 57 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index ba175f9..90d193e 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -746,6 +746,40 @@ static int open_snapshot_device(struct setterm_control *ctl)
 	return fd;
 }
 
+static void set_blanking(struct setterm_control *ctl)
+{
+	char ioctlarg;
+	int ret;
+
+	if (0 <= ctl->opt_bl_min) {
+		printf("\033[9;%d]", ctl->opt_bl_min);
+		return;
+	}
+	switch (ctl->opt_bl_min) {
+	case BLANKSCREEN:
+		ioctlarg = TIOCL_BLANKSCREEN;
+		if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
+			warn(_("cannot force blank"));
+		break;
+	case UNBLANKSCREEN:
+		ioctlarg = TIOCL_UNBLANKSCREEN;
+		if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
+			warn(_("cannot force unblank"));
+		break;
+	case BLANKEDSCREEN:
+		ioctlarg = TIOCL_BLANKEDSCREEN;
+		ret = ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg);
+		if (ret < 0)
+			warn(_("cannot get blank status"));
+		else
+			printf("%d\n", ret);
+		break;
+	default:		/* should be impossible to reach */
+		abort();
+	}
+	return;
+}
+
 static void screendump(struct setterm_control *ctl)
 {
 	unsigned char header[4];
@@ -973,27 +1007,8 @@ static void perform_sequence(struct setterm_control *ctl)
 	}
 
 	/* -blank [0-60]. */
-	if (ctl->opt_blank && vc_only(ctl, "-blank")) {
-		if (ctl->opt_bl_min >= 0)
-			printf("\033[9;%d]", ctl->opt_bl_min);
-		else if (ctl->opt_bl_min == BLANKSCREEN) {
-			char ioctlarg = TIOCL_BLANKSCREEN;
-			if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
-				warn(_("cannot force blank"));
-		} else if (ctl->opt_bl_min == UNBLANKSCREEN) {
-			char ioctlarg = TIOCL_UNBLANKSCREEN;
-			if (ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg))
-				warn(_("cannot force unblank"));
-		} else if (ctl->opt_bl_min == BLANKEDSCREEN) {
-			char ioctlarg = TIOCL_BLANKEDSCREEN;
-			int ret;
-			ret = ioctl(STDIN_FILENO, TIOCLINUX, &ioctlarg);
-			if (ret < 0)
-				warn(_("cannot get blank status"));
-			else
-				printf("%d\n", ret);
-		}
-	}
+	if (ctl->opt_blank && vc_only(ctl, "-blank"))
+		set_blanking(ctl);
 
 	/* -powersave [on|vsync|hsync|powerdown|off] (console) */
 	if (ctl->opt_powersave) {
-- 
1.9.2

Re: [PATCH 03/17] setterm: use string utils to numeric parsing

[Karel Zak]

On Sun, May 18, 2014 at 03:05:27PM +0100, Sami Kerola wrote:
> +static int strtoint_or_err(const char *str)
> +{
> +	int64_t num = strtos64_or_err(str, _("argument error"));
> +
> +	if (num < INT_MIN || INT_MAX < num)
> +		errx(EXIT_FAILURE, _("argument error: %s"), str);
> +	return num;
> +}

 Why not strtos32_or_err() from lib/strutils.c ?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 02/17] setterm: use getopt_long_only() for option parsing

[Karel Zak]

On Sun, May 18, 2014 at 03:05:26PM +0100, Sami Kerola wrote:
> +static void set_opt_flag(int *opt)
> +{
> +	if (*opt)
> +		errx(EXIT_FAILURE, "duplicate use of an option");

 NLS.. _("duplicate use of an option")

> +	*opt = 1;
> +}

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 06/17] setterm: add option control structure

[Karel Zak]

On Sun, May 18, 2014 at 03:05:30PM +0100, Sami Kerola wrote:
> +			ctl->opt_repeat = set_opt_flag(ctl->opt_repeat);
> +			ctl->opt_rep_on = parse_switch(optarg, "on", "off");
...
> +			ctl->opt_foreground = set_opt_flag(ctl->opt_foreground);
> +			ctl->opt_fo_color = parse_febg_color(optarg);

It would be nice (by another patch!) consolidate the names of the
options. For example opt_foreground and opt_fo_color is brain dead.
What about:

     ctl->opt_repeat
     ctl->opt_repeat_on

and

     ctl->opt_foreground
     ctl->opt_foreground_color

etc. Maybe the whole "opt_" prefix is unnecessary.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 06/17] setterm: add option control structure

[Karel Zak]

On Sun, May 18, 2014 at 03:05:30PM +0100, Sami Kerola wrote:
> +int main(int argc, char **argv)
> +{
> +	struct setterm_control ctl;
> +	int term_errno;
>  
> +	memset(&ctl, 0, sizeof (ctl));

The memset() call is unnecessary. It's enough to set any struct member
to zeroize whole struct, etc:


  struct setterm_control ctl = { .opt_te_terminal_name = NULL };


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 09/17] setterm: remove devfs and /dev/vcsa0 support

[Karel Zak]

On Sun, May 18, 2014 at 03:05:33PM +0100, Sami Kerola wrote:
> The devfs files /dev/vcc/a* does not need to be supported, and vcsa0 has
> not existed in years if ever.
> 
> Reference: http://lwn.net/Articles/65197/
> Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devices.txt?id=14186fea0cb06bc43181ce239efe0df6f1af260a#n260
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  bash-completion/setterm |  2 +-
>  term-utils/setterm.c    | 26 +++++++-------------------
>  2 files changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/bash-completion/setterm b/bash-completion/setterm
> index a7ef6eb..f47debf 100644
> --- a/bash-completion/setterm
> +++ b/bash-completion/setterm
> @@ -40,7 +40,7 @@ _setterm_module()
>  			;;
>  		'-dump'|'-append')
>  			local NUM_CONS
> -			NUM_CONS=(/sys/class/tty/*)
> +			NUM_CONS=(/dev/vcsa?*)
>  			COMPREPLY=( $(compgen -W "{1..${#NUM_CONS[*]}}" -- $cur) )
>  			return 0
>  			;;
> diff --git a/term-utils/setterm.c b/term-utils/setterm.c
> index d889db4..3a02347 100644
> --- a/term-utils/setterm.c
> +++ b/term-utils/setterm.c
> @@ -724,31 +724,18 @@ static void show_tabs(void)
>  
>  static void __attribute__((__noreturn__)) read_error(struct setterm_control *ctl)
>  {
> -	if (ctl->opt_sn_num != 0)
> -		errx(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
> -	else
> -		errx(EXIT_DUMPFILE, _("Couldn't read neither /dev/vcsa0 nor /dev/vcsa"));
> +	err(EXIT_DUMPFILE, _("Couldn't read %s"), ctl->in_device);
>  }

 Sooo.. do we really need read_error()? ;-)

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 11/17] setterm: correct usage() bright color argument

[Karel Zak]

On Sun, May 18, 2014 at 03:05:35PM +0100, Sami Kerola wrote:
>  	fputs(_(" -foreground <default|black|blue|cyan|green|magenta|red|white|yellow>\n"), out);
>  	fputs(_(" -background <default|black|blue|cyan|green|magenta|red|white|yellow>\n"), out);
> -	fputs(_(" -ulcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> -	fputs(_(" -ulcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> -	fputs(_(" -hbcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> -	fputs(_(" -hbcolor <black|blue|bright|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> +	fputs(_(" -ulcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> +	fputs(_(" -ulcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> +	fputs(_(" -hbcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);
> +	fputs(_(" -hbcolor <bright> <black|blue|cyan|green|grey|magenta|red|white|yellow>\n"), out);

 too many duplicate information, it would be better to use 

     _(" -foreground <default>|<color>\n")
     ...
     _(" -hbcolor <bright> <color>\n")

 and describe below in usage():

    _(" <color> means black, blue, ...")


 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 13/17] setterm: tell user when options does not effect

[Karel Zak]

On Sun, May 18, 2014 at 03:05:37PM +0100, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  term-utils/setterm.c | 83 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/term-utils/setterm.c b/term-utils/setterm.c
> index f1ed862..2a0162e 100644
> --- a/term-utils/setterm.c
> +++ b/term-utils/setterm.c
> @@ -792,6 +792,17 @@ static void screendump(struct setterm_control *ctl)
>  	return;
>  }
>  
> +/* Some options are applicable when terminal is virtual console. */
> +static int vc_only(struct setterm_control *ctl, const char *err)
> +{
> +	if (!ctl->vcterm) {
> +		if (err)
> +			warnx(_("terminal %s does not support %s"),
> +			      ctl->opt_te_terminal_name, err);
> +	}
> +	return ctl->vcterm;
> +}
> +
>  static void perform_sequence(struct setterm_control *ctl)
>  {
>  	int result;
> @@ -812,49 +823,45 @@ static void perform_sequence(struct setterm_control *ctl)
>  			putp(ti_entry("civis"));
>  	}
>  
> -	/* -linewrap [on|off]. Vc only (vt102) */
> -	if (ctl->opt_linewrap && ctl->vcterm)
> +	/* -linewrap [on|off]. */
> +	if (ctl->opt_linewrap && vc_only(ctl, "-linewrap"))
>  		fputs(ctl->opt_li_on ? "\033[?7h" : "\033[?7l", stdout);
>  
> -	/* -repeat [on|off]. Vc only (vt102) */
> -	if (ctl->opt_repeat && ctl->vcterm)
> +	/* -repeat [on|off]. */
> +	if (ctl->opt_repeat && vc_only(ctl, "-repeat"))

 Please use complete long options in error messages ("--repeat" rather 
 than "-repeat").

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 00/17] pull: almost complete setterm refactoring

[Sami Kerola]

On 18 May 2014 15:05, Sami Kerola <kerolasa@iki.fi> wrote:
> Sami Kerola (17):
>   setterm: clean up includes
>   setterm: use getopt_long_only() for option parsing
>   setterm: use string utils to numeric parsing
>   setterm: move show_tabs() and screendump() functions
>   setterm: remove usage comment segment
>   setterm: add option control structure
>   setterm: add init_terminal() to make main() shorter
>   setterm: clean up screendump()
>   setterm: remove devfs and /dev/vcsa0 support
>   setterm: make -msglevel 0 to work as is did earlier
>   setterm: correct usage() bright color argument
>   setterm: improve perform_sequence() coding style
>   setterm: tell user when options does not effect
>   setterm: improve error messages
>   setterm: various visual terminal effects are not console specific
>   setterm: mark some options to be exclusive with each other
>   setterm: add set_blanking() action

Hi Karel & thank you for feedback, here are my collected improvements.

0002 setterm-use-getopt_long_only-for-option-parsing
> +             errx(EXIT_FAILURE, "duplicate use of an option");
> NLS.. _("duplicate use of an option")

Fixed.


0003 setterm-use-string-utils-to-numeric-parsing
> Why not strtos32_or_err() from lib/strutils.c ?

Because I did not think.  Fixed.


0006 setterm-add-option-control-structure
> +     memset(&ctl, 0, sizeof (ctl));
>
> The memset() call is unnecessary. It's enough to set any struct member
> to zeroize whole struct, etc:
>
>   struct setterm_control ctl = { .opt_te_terminal_name = NULL };

I did not know, and then I went to find out more.  Universal zero
initializer seem to be an option many recommend, so I'll switch to this
format.

  struct setterm_control ctl = { 0 };

This is what the standard tells, just in case someone finds util-linux
mail archive and needs a reference.

http://c0x.coding-guidelines.com/6.7.8.html


0009 setterm-remove-devfs-and-dev-vcsa0-support
>  Sooo.. do we really need read_error()? ;-)

Yup, not needed.


0011 setterm-correct-usage-bright-color-argument
> too many duplicate information, it would be better to use
>
>     _(" -foreground <default>|<color>\n")
>     ...
>     _(" -hbcolor <bright> <color>\n")
>
> and describe below in usage():
>
>    _(" <color> means black, blue, ...")

Fixed. The fix has tiny usage() error. It gives impression grey might
work as --foreground color, but it does send an error. Oh well, users
will need to read friendly manual if they are confused.


0013 setterm-tell-user-when-options-does-not-effect
> Please use complete long options in error messages ("--repeat" rather
> than "-repeat").

Fixed.  And if the the errors are expected too have double dash long
options so should the usage().  That is done in commit below.

https://github.com/kerolasa/lelux-utiliteetit/commit/fb27f91cac9702ad1858d782dd840d5868547423

Here are the updated pull details.  I assume the release v2.25 is done
before merge, and if so I will keep on rebasing the changes until the
merge.

  git://github.com/kerolasa/lelux-utiliteetit.git 2014wk19

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/17] pull: almost complete setterm refactoring

[Benno Schulenberg]

Some day Karel Zak wrote:
> >     _(" -hbcolor <bright> <color>\n")
> >
> > and describe below in usage():
> >
> >    _(" <color> means black, blue, ...")

But but but... the word "bright" is not a placeholder, it must not
be replaced by something else, as <color> by blue, for example.
It is a literal argument, so should not take <>, and it is optional,
so it should take [].  No?

Benno

-- 
http://www.fastmail.fm - A no graphics, no pop-ups email service

Re: [PATCH 00/17] pull: almost complete setterm refactoring

[Karel Zak]

On Tue, May 20, 2014 at 10:11:49AM +0200, Benno Schulenberg wrote:
> 
> Some day Karel Zak wrote:
> > >     _(" -hbcolor <bright> <color>\n")
> > >
> > > and describe below in usage():
> > >
> > >    _(" <color> means black, blue, ...")
> 
> But but but... the word "bright" is not a placeholder, it must not
> be replaced by something else, as <color> by blue, for example.
> It is a literal argument, so should not take <>, and it is optional,
> so it should take [].  No

 Yep, fixed. Thanks!

 BTW, setterm still need our love:
 
  * usage() contains only options, but no any description

  * it would be nice to have long options (--term rather than -term)
    in the man page

  * nowhere (in man page) is info that --long options are supported
    since v2.25 (and for backward compatibility is better to use
    "-foo" rather than "--foo")


    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com