[PATCH 0/4] a few usage cleanups (--help, etc)

[PATCH 0/4] a few usage cleanups (--help, etc)

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

This patch-set corrects some minor issues for flock, agetty and gettopt.
If this is acceptable I would send similiar patches for the other
commands too.

You may also squash these commits to have less noise in the git history.

Ruediger Meier (4):
  toos: add checkusage.sh
  flock: cleaup --help
  getopt: cleanup --help
  agetty: cleanup --help

 Makefile.am         |   5 +++
 misc-utils/getopt.c |  48 ++++++++++++-------------
 sys-utils/flock.c   |  67 +++++++++++++++++-----------------
 term-utils/agetty.c | 102 ++++++++++++++++++++++++++--------------------------
 tools/checkusage.sh |  74 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 186 insertions(+), 110 deletions(-)
 create mode 100755 tools/checkusage.sh

-- 
1.8.5.6

[PATCH 1/4] toos: add checkusage.sh

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Just some simple generic tests for our UL commands, regarding options --help,
--version and --unknownopt.

For the record here are the current candidates with possible problems:

$ make checkusage
agetty: --unknownopt, stderr too long: 45
blockdev: --unknownopt, stderr too long: 28
flock: --help, no stdout
flock: --help, non-empty stderr
fsck: --help, returns error
fsck: --version, returns error
fsck: --unknownopt, non-empty stdout
fsck: --unknownopt, stderr too long: 18
getopt: --help, returns error
kill: --unknownopt, stderr too short: 1
login: --help, returns error
login: --version, returns error
login: --unknownopt, stderr too long: 4
lsipc: --unknownopt, stderr too long: 77
mkfs: --unknownopt, stderr too long: 8
mkfs.cramfs: --help, returns error
mkfs.cramfs: --version, returns error
more: --help, returns error
more: --version, returns error
more: --unknownopt, stderr too long: 21
nologin: --help, returns error
nologin: --version, returns error
pg: --unknownopt, stderr too long: 23
renice: --unknownopt, stderr too long: 18
rtcwake: --unknownopt, non-empty stdout
rtcwake: --unknownopt, stderr too long: 21
sulogin: --unknownopt, stderr too long: 17
whereis: --help, returns error
whereis: --version, returns error
whereis: --unknownopt, stderr too long: 18
write: --unknownopt, stderr too long: 12

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 Makefile.am         |  5 ++++
 tools/checkusage.sh | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100755 tools/checkusage.sh

diff --git a/Makefile.am b/Makefile.am
index 0bfc27f..325d1f4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -183,6 +183,11 @@ checkdecl:
 checkcompletion:
 	@ $(top_srcdir)/tools/checkcompletion.sh $(top_srcdir)
 
+checkusage:
+	@ $(top_srcdir)/tools/checkusage.sh \
+		$(bin_PROGRAMS) $(sbin_PROGRAMS) \
+		$(usrbin_exec_PROGRAMS) $(usrsbin_exec_PROGRAMS)
+
 DISTCHECK_CONFIGURE_FLAGS = \
 	--disable-use-tty-group \
 	--disable-silent-rules \
diff --git a/tools/checkusage.sh b/tools/checkusage.sh
new file mode 100755
index 0000000..3f8cc0f
--- /dev/null
+++ b/tools/checkusage.sh
@@ -0,0 +1,74 @@
+#!/bin/bash
+
+export LC_ALL=C
+
+if [ "$#" -lt 1 ]; then
+	echo "usage: $0 program..." >&2
+	echo "       or try 'make checkusage' to check all built programs" >&2
+	exit 1
+fi
+
+builddir="."
+cmds=$(echo $@ | tr ' ' '\n' | sort)
+
+function exec_option {
+	local cmd=$1
+	shift
+
+	opt=$@
+	out=$("$cmd" "$@" 2>/dev/null)
+	err=$("$cmd" "$@" 2>&1 >/dev/null)
+	ret=$?
+}
+
+for c in $cmds; do
+	cc="$builddir/$c"
+	if ! type "$cc" &>/dev/null; then
+		echo "$c: does not exist"
+		continue
+	fi
+
+	exec_option "$cc" --help
+	if test $ret != 0; then
+		echo "$c: $opt, returns error"
+	else
+		if test -z "$out"; then
+			echo "$c: $opt, no stdout"
+		fi
+		if test -n "$err"; then
+			echo "$c: $opt, non-empty stderr"
+		fi
+	fi
+
+	exec_option "$cc" --version
+	if test $ret != 0; then
+		echo "$c: $opt, returns error"
+	else
+		if test -z "$out"; then
+			echo "$c: $opt, no stdout"
+		fi
+		if test -n "$err"; then
+			echo "$c: $opt, non-empty stderr"
+		fi
+	fi
+
+	exec_option "$cc" --unknownopt
+	if test $ret = 0; then
+		echo "$c: $opt, returns no error"
+	fi
+	if test -n "$out"; then
+		echo "$c: $opt, non-empty stdout"
+	fi
+	if test -z "$err"; then
+		echo "$c: $opt, no stderr"
+	else
+		out_len=$(echo "$out" | wc -l)
+		err_len=$(echo "$err" | wc -l)
+		if test "$err_len" -gt 2; then
+			echo "$c: $opt, stderr too long: $err_len"
+		elif test "$err_len" -lt 2; then
+			echo "$c: $opt, stderr too short: $err_len"
+		fi
+	fi
+done
+
-- 
1.8.5.6

[PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Write --help always to stdout.
Always use errtryhelp(EX_USAGE) instead of errx(EX_USAGE, ...)

Noticed by

$ ./tools/checkusage.sh flock
flock: help no stdout
flock: help non-empty stderr

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 sys-utils/flock.c | 67 +++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/sys-utils/flock.c b/sys-utils/flock.c
index 50194bd..5eab1db 100644
--- a/sys-utils/flock.c
+++ b/sys-utils/flock.c
@@ -46,34 +46,34 @@
 #include "monotonic.h"
 #include "timer.h"
 
-static void __attribute__((__noreturn__)) usage(int ex)
+static void __attribute__((__noreturn__)) usage(void)
 {
-	fprintf(stderr, USAGE_HEADER);
-	fprintf(stderr,
+	printf(USAGE_HEADER);
+	printf(
 		_(" %1$s [options] <file>|<directory> <command> [<argument>...]\n"
 		  " %1$s [options] <file>|<directory> -c <command>\n"
 		  " %1$s [options] <file descriptor number>\n"),
 		program_invocation_short_name);
 
-	fputs(USAGE_SEPARATOR, stderr);
-	fputs(_("Manage file locks from shell scripts.\n"), stderr);
-
-	fputs(USAGE_OPTIONS, stderr);
-	fputs(_(  " -s, --shared             get a shared lock\n"), stderr);
-	fputs(_(  " -x, --exclusive          get an exclusive lock (default)\n"), stderr);
-	fputs(_(  " -u, --unlock             remove a lock\n"), stderr);
-	fputs(_(  " -n, --nonblock           fail rather than wait\n"), stderr);
-	fputs(_(  " -w, --timeout <secs>     wait for a limited amount of time\n"), stderr);
-	fputs(_(  " -E, --conflict-exit-code <number>  exit code after conflict or timeout\n"), stderr);
-	fputs(_(  " -o, --close              close file descriptor before running command\n"), stderr);
-	fputs(_(  " -c, --command <command>  run a single command string through the shell\n"), stderr);
-	fputs(_(  " -F, --no-fork            execute command without forking\n"), stderr);
-	fputs(_(  "     --verbose            increase verbosity\n"), stderr);
-	fprintf(stderr, USAGE_SEPARATOR);
-	fprintf(stderr, USAGE_HELP);
-	fprintf(stderr, USAGE_VERSION);
-	fprintf(stderr, USAGE_MAN_TAIL("flock(1)"));
-	exit(ex);
+	puts(USAGE_SEPARATOR);
+	puts(_("Manage file locks from shell scripts.\n"));
+
+	puts(USAGE_OPTIONS);
+	puts(_(  " -s, --shared             get a shared lock\n"));
+	puts(_(  " -x, --exclusive          get an exclusive lock (default)\n"));
+	puts(_(  " -u, --unlock             remove a lock\n"));
+	puts(_(  " -n, --nonblock           fail rather than wait\n"));
+	puts(_(  " -w, --timeout <secs>     wait for a limited amount of time\n"));
+	puts(_(  " -E, --conflict-exit-code <number>  exit code after conflict or timeout\n"));
+	puts(_(  " -o, --close              close file descriptor before running command\n"));
+	puts(_(  " -c, --command <command>  run a single command string through the shell\n"));
+	puts(_(  " -F, --no-fork            execute command without forking\n"));
+	puts(_(  "     --verbose            increase verbosity\n"));
+	printf(USAGE_SEPARATOR);
+	printf(USAGE_HELP);
+	printf(USAGE_VERSION);
+	printf(USAGE_MAN_TAIL("flock(1)"));
+	exit(EXIT_SUCCESS);
 }
 
 static sig_atomic_t timeout_expired = 0;
@@ -170,9 +170,6 @@ int main(int argc, char *argv[])
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	if (argc < 2)
-		usage(EX_USAGE);
-
 	memset(&timeout, 0, sizeof timeout);
 
 	optopt = 0;
@@ -215,24 +212,25 @@ int main(int argc, char *argv[])
 			printf(UTIL_LINUX_VERSION);
 			exit(EX_OK);
 		case 'h':
-			usage(0);
+			usage();
 		default:
 			errtryhelp(EX_USAGE);
 		}
 	}
 
-	if (no_fork && do_close)
-		errx(EX_USAGE,
-			_("the --no-fork and --close options are incompatible"));
-
+	if (no_fork && do_close) {
+		warnx(_("the --no-fork and --close options are incompatible"));
+		errtryhelp(EX_USAGE);
+	}
 	if (argc > optind + 1) {
 		/* Run command */
 		if (!strcmp(argv[optind + 1], "-c") ||
 		    !strcmp(argv[optind + 1], "--command")) {
-			if (argc != optind + 3)
-				errx(EX_USAGE,
-				     _("%s requires exactly one command argument"),
+			if (argc != optind + 3) {
+				warnx(_("%s requires exactly one command argument"),
 				     argv[optind + 1]);
+				errtryhelp(EX_USAGE);
+			}
 			cmd_argv = sh_c_argv;
 			cmd_argv[0] = getenv("SHELL");
 			if (!cmd_argv[0] || !*cmd_argv[0])
@@ -252,7 +250,8 @@ int main(int argc, char *argv[])
 		fd = strtos32_or_err(argv[optind], _("bad file descriptor"));
 	} else {
 		/* Bad options */
-		errx(EX_USAGE, _("requires file descriptor, file or directory"));
+		warnx(_("requires file descriptor, file or directory"));
+		errtryhelp(EX_USAGE);
 	}
 
 	if (have_timeout) {
-- 
1.8.5.6

[PATCH 3/4] getopt: cleanup usage issues

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Fixed checkusage.sh warnings:
  getopt: --help, returns error
  getopt: --help, no stdout
  getopt: --help, non-empty stderr

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 misc-utils/getopt.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/misc-utils/getopt.c b/misc-utils/getopt.c
index af4cf38..e7c549b 100644
--- a/misc-utils/getopt.c
+++ b/misc-utils/getopt.c
@@ -243,9 +243,7 @@ static void __attribute__ ((__noreturn__)) parse_error(const char *message)
 {
 	if (message)
 		warnx("%s", message);
-	fprintf(stderr, _("Try `%s --help' for more information.\n"),
-		program_invocation_short_name);
-	exit(PARAMETER_EXIT_CODE);
+	errtryhelp(PARAMETER_EXIT_CODE);
 }
 
 
@@ -325,33 +323,33 @@ static shell_t shell_type(const char *new_shell)
 	parse_error(_("unknown shell after -s or --shell argument"));
 }
 
-static void __attribute__ ((__noreturn__)) print_help(void)
+static void __attribute__ ((__noreturn__)) usage(void)
 {
-	fputs(USAGE_HEADER, stderr);
-	fprintf(stderr, _(
+	puts(USAGE_HEADER);
+	printf(_(
 		" %1$s <optstring> <parameters>\n"
 		" %1$s [options] [--] <optstring> <parameters>\n"
 		" %1$s [options] -o|--options <optstring> [options] [--] <parameters>\n"),
 		program_invocation_short_name);
 
-	fputs(USAGE_SEPARATOR, stderr);
-	fputs(_("Parse command options.\n"), stderr);
-
-	fputs(USAGE_OPTIONS, stderr);
-	fputs(_(" -a, --alternative             allow long options starting with single -\n"), stderr);
-	fputs(_(" -l, --longoptions <longopts>  the long options to be recognized\n"), stderr);
-	fputs(_(" -n, --name <progname>         the name under which errors are reported\n"), stderr);
-	fputs(_(" -o, --options <optstring>     the short options to be recognized\n"), stderr);
-	fputs(_(" -q, --quiet                   disable error reporting by getopt(3)\n"), stderr);
-	fputs(_(" -Q, --quiet-output            no normal output\n"), stderr);
-	fputs(_(" -s, --shell <shell>           set quoting conventions to those of <shell>\n"), stderr);
-	fputs(_(" -T, --test                    test for getopt(1) version\n"), stderr);
-	fputs(_(" -u, --unquoted                do not quote the output\n"), stderr);
-	fputs(USAGE_SEPARATOR, stderr);
-	fputs(USAGE_HELP, stderr);
-	fputs(USAGE_VERSION, stderr);
-	fprintf(stderr, USAGE_MAN_TAIL("getopt(1)"));
-	exit(PARAMETER_EXIT_CODE);
+	puts(USAGE_SEPARATOR);
+	puts(_("Parse command options.\n"));
+
+	puts(USAGE_OPTIONS);
+	puts(_(" -a, --alternative             allow long options starting with single -\n"));
+	puts(_(" -l, --longoptions <longopts>  the long options to be recognized\n"));
+	puts(_(" -n, --name <progname>         the name under which errors are reported\n"));
+	puts(_(" -o, --options <optstring>     the short options to be recognized\n"));
+	puts(_(" -q, --quiet                   disable error reporting by getopt(3)\n"));
+	puts(_(" -Q, --quiet-output            no normal output\n"));
+	puts(_(" -s, --shell <shell>           set quoting conventions to those of <shell>\n"));
+	puts(_(" -T, --test                    test for getopt(1) version\n"));
+	puts(_(" -u, --unquoted                do not quote the output\n"));
+	puts(USAGE_SEPARATOR);
+	puts(USAGE_HELP);
+	puts(USAGE_VERSION);
+	printf(USAGE_MAN_TAIL("getopt(1)"));
+	exit(EXIT_SUCCESS);
 }
 
 int main(int argc, char *argv[])
@@ -417,7 +415,7 @@ int main(int argc, char *argv[])
 			getopt_long_fp = getopt_long_only;
 			break;
 		case 'h':
-			print_help();
+			usage();
 		case 'o':
 			free(ctl.optstr);
 			ctl.optstr = xstrdup(optarg);
-- 
1.8.5.6

[PATCH 4/4] agetty: cleanup usage issues

[Ruediger Meier]

From: Ruediger Meier <ruediger.meier@ga-group.nl>

Fixed checkusage.sh warnings:
  agetty: --unknownopt, stderr too long: 45

Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
---
 term-utils/agetty.c | 102 ++++++++++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 938fa2f..542cbce 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -308,7 +308,7 @@ static void termio_final(struct options *op,
 			 struct termios *tp, struct chardata *cp);
 static int caps_lock(char *s);
 static speed_t bcode(char *s);
-static void usage(FILE * out) __attribute__((__noreturn__));
+static void usage(void) __attribute__((__noreturn__));
 static void log_err(const char *, ...) __attribute__((__noreturn__))
 			       __attribute__((__format__(printf, 1, 2)));
 static void log_warn (const char *, ...)
@@ -785,9 +785,9 @@ static void parse_args(int argc, char **argv, struct options *op)
 			printf(UTIL_LINUX_VERSION);
 			exit(EXIT_SUCCESS);
 		case HELP_OPTION:
-			usage(stdout);
+			usage();
 		default:
-			usage(stderr);
+			errtryhelp(EXIT_FAILURE);
 		}
 	}
 
@@ -795,7 +795,7 @@ static void parse_args(int argc, char **argv, struct options *op)
 
 	if (argc < optind + 1) {
 		log_warn(_("not enough arguments"));
-		usage(stderr);
+		errtryhelp(EXIT_FAILURE);
 	}
 
 	/* Accept "tty", "baudrate tty", and "tty baudrate". */
@@ -804,7 +804,7 @@ static void parse_args(int argc, char **argv, struct options *op)
 		parse_speeds(op, argv[optind++]);
 		if (argc < optind + 1) {
 			warn(_("not enough arguments"));
-			usage(stderr);
+			errtryhelp(EXIT_FAILURE);
 		}
 		op->tty = argv[optind++];
 	} else {
@@ -2106,53 +2106,53 @@ static speed_t bcode(char *s)
 	return 0;
 }
 
-static void __attribute__ ((__noreturn__)) usage(FILE *out)
+static void __attribute__ ((__noreturn__)) usage(void)
 {
-	fputs(USAGE_HEADER, out);
-	fprintf(out, _(" %1$s [options] <line> [<baud_rate>,...] [<termtype>]\n"
-		       " %1$s [options] <baud_rate>,... <line> [<termtype>]\n"), program_invocation_short_name);
-
-	fputs(USAGE_SEPARATOR, out);
-	fputs(_("Open a terminal and set its mode.\n"), out);
-
-	fputs(USAGE_OPTIONS, out);
-	fputs(_(" -8, --8bits                assume 8-bit tty\n"), out);
-	fputs(_(" -a, --autologin <user>     login the specified user automatically\n"), out);
-	fputs(_(" -c, --noreset              do not reset control mode\n"), out);
-	fputs(_(" -E, --remote               use -r <hostname> for login(1)\n"), out);
-	fputs(_(" -f, --issue-file <file>    display issue file\n"), out);
-	fputs(_(" -h, --flow-control         enable hardware flow control\n"), out);
-	fputs(_(" -H, --host <hostname>      specify login host\n"), out);
-	fputs(_(" -i, --noissue              do not display issue file\n"), out);
-	fputs(_(" -I, --init-string <string> set init string\n"), out);
-	fputs(_(" -J  --noclear              do not clear the screen before prompt\n"), out);
-	fputs(_(" -l, --login-program <file> specify login program\n"), out);
-	fputs(_(" -L, --local-line[=<mode>]  control the local line flag\n"), out);
-	fputs(_(" -m, --extract-baud         extract baud rate during connect\n"), out);
-	fputs(_(" -n, --skip-login           do not prompt for login\n"), out);
-	fputs(_(" -N  --nonewline            do not print a newline before issue\n"), out);
-	fputs(_(" -o, --login-options <opts> options that are passed to login\n"), out);
-	fputs(_(" -p, --login-pause          wait for any key before the login\n"), out);
-	fputs(_(" -r, --chroot <dir>         change root to the directory\n"), out);
-	fputs(_(" -R, --hangup               do virtually hangup on the tty\n"), out);
-	fputs(_(" -s, --keep-baud            try to keep baud rate after break\n"), out);
-	fputs(_(" -t, --timeout <number>     login process timeout\n"), out);
-	fputs(_(" -U, --detect-case          detect uppercase terminal\n"), out);
-	fputs(_(" -w, --wait-cr              wait carriage-return\n"), out);
-	fputs(_("     --nohints              do not print hints\n"), out);
-	fputs(_("     --nohostname           no hostname at all will be shown\n"), out);
-	fputs(_("     --long-hostname        show full qualified hostname\n"), out);
-	fputs(_("     --erase-chars <string> additional backspace chars\n"), out);
-	fputs(_("     --kill-chars <string>  additional kill chars\n"), out);
-	fputs(_("     --chdir <directory>    chdir before the login\n"), out);
-	fputs(_("     --delay <number>       sleep seconds before prompt\n"), out);
-	fputs(_("     --nice <number>        run login with this priority\n"), out);
-	fputs(_("     --reload               reload prompts on running agetty instances\n"), out);
-	fputs(_("     --help                 display this help and exit\n"), out);
-	fputs(_("     --version              output version information and exit\n"), out);
-	fprintf(out, USAGE_MAN_TAIL("agetty(8)"));
-
-	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
+	puts(USAGE_HEADER);
+	printf(_(" %1$s [options] <line> [<baud_rate>,...] [<termtype>]\n"
+		     " %1$s [options] <baud_rate>,... <line> [<termtype>]\n"), program_invocation_short_name);
+
+	puts(USAGE_SEPARATOR);
+	puts(_("Open a terminal and set its mode.\n"));
+
+	puts(USAGE_OPTIONS);
+	puts(_(" -8, --8bits                assume 8-bit tty\n"));
+	puts(_(" -a, --autologin <user>     login the specified user automatically\n"));
+	puts(_(" -c, --noreset              do not reset control mode\n"));
+	puts(_(" -E, --remote               use -r <hostname> for login(1)\n"));
+	puts(_(" -f, --issue-file <file>    display issue file\n"));
+	puts(_(" -h, --flow-control         enable hardware flow control\n"));
+	puts(_(" -H, --host <hostname>      specify login host\n"));
+	puts(_(" -i, --noissue              do not display issue file\n"));
+	puts(_(" -I, --init-string <string> set init string\n"));
+	puts(_(" -J  --noclear              do not clear the screen before prompt\n"));
+	puts(_(" -l, --login-program <file> specify login program\n"));
+	puts(_(" -L, --local-line[=<mode>]  control the local line flag\n"));
+	puts(_(" -m, --extract-baud         extract baud rate during connect\n"));
+	puts(_(" -n, --skip-login           do not prompt for login\n"));
+	puts(_(" -N  --nonewline            do not print a newline before issue\n"));
+	puts(_(" -o, --login-options <opts> options that are passed to login\n"));
+	puts(_(" -p, --login-pause          wait for any key before the login\n"));
+	puts(_(" -r, --chroot <dir>         change root to the directory\n"));
+	puts(_(" -R, --hangup               do virtually hangup on the tty\n"));
+	puts(_(" -s, --keep-baud            try to keep baud rate after break\n"));
+	puts(_(" -t, --timeout <number>     login process timeout\n"));
+	puts(_(" -U, --detect-case          detect uppercase terminal\n"));
+	puts(_(" -w, --wait-cr              wait carriage-return\n"));
+	puts(_("     --nohints              do not print hints\n"));
+	puts(_("     --nohostname           no hostname at all will be shown\n"));
+	puts(_("     --long-hostname        show full qualified hostname\n"));
+	puts(_("     --erase-chars <string> additional backspace chars\n"));
+	puts(_("     --kill-chars <string>  additional kill chars\n"));
+	puts(_("     --chdir <directory>    chdir before the login\n"));
+	puts(_("     --delay <number>       sleep seconds before prompt\n"));
+	puts(_("     --nice <number>        run login with this priority\n"));
+	puts(_("     --reload               reload prompts on running agetty instances\n"));
+	puts(_("     --help                 display this help and exit\n"));
+	puts(_("     --version              output version information and exit\n"));
+	printf(USAGE_MAN_TAIL("agetty(8)"));
+
+	exit(EXIT_SUCCESS);
 }
 
 /*
-- 
1.8.5.6

Re: [PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

On Monday 19 June 2017, Ruediger Meier wrote:
> From: Ruediger Meier <ruediger.meier@ga-group.nl>
>
> Write --help always to stdout.
> Always use errtryhelp(EX_USAGE) instead of errx(EX_USAGE, ...)
>
> Noticed by
>
> $ ./tools/checkusage.sh flock
> flock: help no stdout
> flock: help non-empty stderr
>
> Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
> ---
>  sys-utils/flock.c | 67
> +++++++++++++++++++++++++++---------------------------- 1 file
> changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/sys-utils/flock.c b/sys-utils/flock.c
> index 50194bd..5eab1db 100644
> --- a/sys-utils/flock.c
> +++ b/sys-utils/flock.c
> @@ -46,34 +46,34 @@
>  #include "monotonic.h"
>  #include "timer.h"
>
> -static void __attribute__((__noreturn__)) usage(int ex)
> +static void __attribute__((__noreturn__)) usage(void)
>  {
> -	fprintf(stderr, USAGE_HEADER);
> -	fprintf(stderr,
> +	printf(USAGE_HEADER);
> +	printf(
>  		_(" %1$s [options] <file>|<directory> <command> [<argument>...]\n"
>  		  " %1$s [options] <file>|<directory> -c <command>\n"
>  		  " %1$s [options] <file descriptor number>\n"),
>  		program_invocation_short_name);
>
> -	fputs(USAGE_SEPARATOR, stderr);
> -	fputs(_("Manage file locks from shell scripts.\n"), stderr);
> -
> -	fputs(USAGE_OPTIONS, stderr);
> -	fputs(_(  " -s, --shared             get a shared lock\n"),
> stderr); -	fputs(_(  " -x, --exclusive          get an exclusive lock
> (default)\n"), stderr); -	fputs(_(  " -u, --unlock             remove
> a lock\n"), stderr); -	fputs(_(  " -n, --nonblock           fail
> rather than wait\n"), stderr); -	fputs(_(  " -w, --timeout <secs>    
> wait for a limited amount of time\n"), stderr); -	fputs(_(  " -E,
> --conflict-exit-code <number>  exit code after conflict or
> timeout\n"), stderr); -	fputs(_(  " -o, --close              close
> file descriptor before running command\n"), stderr); -	fputs(_(  "
> -c, --command <command>  run a single command string through the
> shell\n"), stderr); -	fputs(_(  " -F, --no-fork            execute
> command without forking\n"), stderr); -	fputs(_(  "     --verbose    
>        increase verbosity\n"), stderr); -	fprintf(stderr,
> USAGE_SEPARATOR);
> -	fprintf(stderr, USAGE_HELP);
> -	fprintf(stderr, USAGE_VERSION);
> -	fprintf(stderr, USAGE_MAN_TAIL("flock(1)"));
> -	exit(ex);
> +	puts(USAGE_SEPARATOR);
> +	puts(_("Manage file locks from shell scripts.\n"));
> +
> +	puts(USAGE_OPTIONS);
> +	puts(_(  " -s, --shared             get a shared lock\n"));
> +	puts(_(  " -x, --exclusive          get an exclusive lock
> (default)\n")); +	puts(_(  " -u, --unlock             remove a
> lock\n"));
> +	puts(_(  " -n, --nonblock           fail rather than wait\n"));
> +	puts(_(  " -w, --timeout <secs>     wait for a limited amount of
> time\n")); +	puts(_(  " -E, --conflict-exit-code <number>  exit code
> after conflict or timeout\n")); +	puts(_(  " -o, --close             
> close file descriptor before running command\n")); +	puts(_(  " -c,
> --command <command>  run a single command string through the
> shell\n")); +	puts(_(  " -F, --no-fork            execute command
> without forking\n")); +	puts(_(  "     --verbose            increase
> verbosity\n")); +	printf(USAGE_SEPARATOR);
> +	printf(USAGE_HELP);
> +	printf(USAGE_VERSION);
> +	printf(USAGE_MAN_TAIL("flock(1)"));
> +	exit(EXIT_SUCCESS);
>  }

Sorry I forgot that the fputs/puts repacement adds another newline.

I could either
  1. remove the newlines from the strings (but would this cause extra
     work for translators?)
  2. use printf instead of puts
  3. don't change anything, just use constant FILE argument stdout.

>  static sig_atomic_t timeout_expired = 0;
> @@ -170,9 +170,6 @@ int main(int argc, char *argv[])
>  	textdomain(PACKAGE);
>  	atexit(close_stdout);
>
> -	if (argc < 2)
> -		usage(EX_USAGE);
> -
>  	memset(&timeout, 0, sizeof timeout);
>
>  	optopt = 0;
> @@ -215,24 +212,25 @@ int main(int argc, char *argv[])
>  			printf(UTIL_LINUX_VERSION);
>  			exit(EX_OK);
>  		case 'h':
> -			usage(0);
> +			usage();
>  		default:
>  			errtryhelp(EX_USAGE);
>  		}
>  	}
>
> -	if (no_fork && do_close)
> -		errx(EX_USAGE,
> -			_("the --no-fork and --close options are incompatible"));
> -
> +	if (no_fork && do_close) {
> +		warnx(_("the --no-fork and --close options are incompatible"));
> +		errtryhelp(EX_USAGE);
> +	}
>  	if (argc > optind + 1) {
>  		/* Run command */
>  		if (!strcmp(argv[optind + 1], "-c") ||
>  		    !strcmp(argv[optind + 1], "--command")) {
> -			if (argc != optind + 3)
> -				errx(EX_USAGE,
> -				     _("%s requires exactly one command argument"),
> +			if (argc != optind + 3) {
> +				warnx(_("%s requires exactly one command argument"),
>  				     argv[optind + 1]);
> +				errtryhelp(EX_USAGE);
> +			}
>  			cmd_argv = sh_c_argv;
>  			cmd_argv[0] = getenv("SHELL");
>  			if (!cmd_argv[0] || !*cmd_argv[0])
> @@ -252,7 +250,8 @@ int main(int argc, char *argv[])
>  		fd = strtos32_or_err(argv[optind], _("bad file descriptor"));
>  	} else {
>  		/* Bad options */
> -		errx(EX_USAGE, _("requires file descriptor, file or directory"));
> +		warnx(_("requires file descriptor, file or directory"));
> +		errtryhelp(EX_USAGE);
>  	}
>
>  	if (have_timeout) {

Re: [PATCH 2/4] flock: cleanup usage issues

[Karel Zak]

On Mon, Jun 19, 2017 at 02:36:17PM +0200, Ruediger Meier wrote:
> Sorry I forgot that the fputs/puts repacement adds another newline.
> 
> I could either
>   1. remove the newlines from the strings (but would this cause extra
>      work for translators?)
>   2. use printf instead of puts
>   3. don't change anything, just use constant FILE argument stdout.

I think fputs(..., stdout) is non-invasive change, better than modify
all strings and probably more effective than printf().

    Karel

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

Re: [PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

On Monday 19 June 2017, Karel Zak wrote:
> On Mon, Jun 19, 2017 at 02:36:17PM +0200, Ruediger Meier wrote:
> > Sorry I forgot that the fputs/puts repacement adds another newline.
> >
> > I could either
> >   1. remove the newlines from the strings (but would this cause
> > extra work for translators?)
> >   2. use printf instead of puts
> >   3. don't change anything, just use constant FILE argument stdout.
>
> I think fputs(..., stdout) is non-invasive change, better than modify
> all strings and probably more effective than printf().
>
>     Karel


I'm using

-static void usage(FILE * out) {
-    FILE * out = stdout;
-    fputs(USAGE_HEADER, out);
+static void usage(void) {
+    FILE * out = stdout;
+    fputs(USAGE_HEADER, out);
    ...
}

This will give the smallest patches when I go through all the other 
commands.

Here it is updated:
https://github.com/karelzak/util-linux/pull/465

cu,
Rudi

Re: [PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

On Monday 19 June 2017, Ruediger Meier wrote:
> On Monday 19 June 2017, Karel Zak wrote:
> > On Mon, Jun 19, 2017 at 02:36:17PM +0200, Ruediger Meier wrote:
> > > Sorry I forgot that the fputs/puts repacement adds another
> > > newline.
> > >
> > > I could either
> > >   1. remove the newlines from the strings (but would this cause
> > > extra work for translators?)
> > >   2. use printf instead of puts
> > >   3. don't change anything, just use constant FILE argument
> > > stdout.
> >
> > I think fputs(..., stdout) is non-invasive change, better than
> > modify all strings and probably more effective than printf().
> >
> >     Karel
>
> I'm using
>
> -static void usage(FILE * out) {
> -    FILE * out = stdout;
> -    fputs(USAGE_HEADER, out);
> +static void usage(void) {
> +    FILE * out = stdout;
> +    fputs(USAGE_HEADER, out);
>     ...
> }

Sorry I mean this 

-static void usage(FILE * out) {
+static void usage(void) {
+    FILE * out = stdout;
     fputs(USAGE_HEADER, out);
     ...
}

> This will give the smallest patches when I go through all the other
> commands.
>
> Here it is updated:
> https://github.com/karelzak/util-linux/pull/465
>
> cu,
> Rudi
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] flock: cleanup usage issues

[Karel Zak]

On Mon, Jun 19, 2017 at 04:40:14PM +0200, Ruediger Meier wrote:
> On Monday 19 June 2017, Ruediger Meier wrote:
> > On Monday 19 June 2017, Karel Zak wrote:
> > > On Mon, Jun 19, 2017 at 02:36:17PM +0200, Ruediger Meier wrote:
> > > > Sorry I forgot that the fputs/puts repacement adds another
> > > > newline.
> > > >
> > > > I could either
> > > >   1. remove the newlines from the strings (but would this cause
> > > > extra work for translators?)
> > > >   2. use printf instead of puts
> > > >   3. don't change anything, just use constant FILE argument
> > > > stdout.
> > >
> > > I think fputs(..., stdout) is non-invasive change, better than
> > > modify all strings and probably more effective than printf().
> > >
> > >     Karel
> >
> > I'm using
> >
> > -static void usage(FILE * out) {
> > -    FILE * out = stdout;
> > -    fputs(USAGE_HEADER, out);
> > +static void usage(void) {
> > +    FILE * out = stdout;
> > +    fputs(USAGE_HEADER, out);
> >     ...
> > }
> 
> Sorry I mean this 
> 
> -static void usage(FILE * out) {
> +static void usage(void) {
> +    FILE * out = stdout;
>      fputs(USAGE_HEADER, out);
>      ...
> }

Yes, seems good, but I sure one day someone will ask: 

    what the hell are you doing with "out = stdout;" ?

:-)

    Karel

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

Re: [PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

On Monday 19 June 2017, Karel Zak wrote:
> On Mon, Jun 19, 2017 at 04:40:14PM +0200, Ruediger Meier wrote:
> > On Monday 19 June 2017, Ruediger Meier wrote:
> > > On Monday 19 June 2017, Karel Zak wrote:
> > > > On Mon, Jun 19, 2017 at 02:36:17PM +0200, Ruediger Meier wrote:
> > > > > Sorry I forgot that the fputs/puts repacement adds another
> > > > > newline.
> > > > >
> > > > > I could either
> > > > >   1. remove the newlines from the strings (but would this
> > > > > cause extra work for translators?)
> > > > >   2. use printf instead of puts
> > > > >   3. don't change anything, just use constant FILE argument
> > > > > stdout.
> > > >
> > > > I think fputs(..., stdout) is non-invasive change, better than
> > > > modify all strings and probably more effective than printf().
> > > >
> > > >     Karel
> > >
> > > I'm using
> > >
> > > -static void usage(FILE * out) {
> > > -    FILE * out = stdout;
> > > -    fputs(USAGE_HEADER, out);
> > > +static void usage(void) {
> > > +    FILE * out = stdout;
> > > +    fputs(USAGE_HEADER, out);
> > >     ...
> > > }
> >
> > Sorry I mean this
> >
> > -static void usage(FILE * out) {
> > +static void usage(void) {
> > +    FILE * out = stdout;
> >      fputs(USAGE_HEADER, out);
> >      ...
> > }
>
> Yes, seems good, but I sure one day someone will ask:
>
>     what the hell are you doing with "out = stdout;" ?
>
> :-)

Hehe I know :)

This is all legacy. It's funny, eventhough I hate usage messages on 
error, my new checkusage.sh script does it also that way. That's how 
most simple commands are born ;)

cu,
Rudi

Re: [PATCH 4/4] agetty: cleanup usage issues

[Ruediger Meier]

On Monday 19 June 2017, Ruediger Meier wrote:
> From: Ruediger Meier <ruediger.meier@ga-group.nl>
>
> Fixed checkusage.sh warnings:
>   agetty: --unknownopt, stderr too long: 45
>
> Signed-off-by: Ruediger Meier <ruediger.meier@ga-group.nl>
> ---
>  term-utils/agetty.c | 102
> ++++++++++++++++++++++++++-------------------------- 1 file changed,
> 51 insertions(+), 51 deletions(-)
>
> diff --git a/term-utils/agetty.c b/term-utils/agetty.c
> index 938fa2f..542cbce 100644
> --- a/term-utils/agetty.c
> +++ b/term-utils/agetty.c
> @@ -308,7 +308,7 @@ static void termio_final(struct options *op,
>  			 struct termios *tp, struct chardata *cp);
>  static int caps_lock(char *s);
>  static speed_t bcode(char *s);
> -static void usage(FILE * out) __attribute__((__noreturn__));
> +static void usage(void) __attribute__((__noreturn__));
>  static void log_err(const char *, ...) __attribute__((__noreturn__))
>  			       __attribute__((__format__(printf, 1, 2)));
>  static void log_warn (const char *, ...)
> @@ -785,9 +785,9 @@ static void parse_args(int argc, char **argv,
> struct options *op) printf(UTIL_LINUX_VERSION);
>  			exit(EXIT_SUCCESS);
>  		case HELP_OPTION:
> -			usage(stdout);
> +			usage();
>  		default:
> -			usage(stderr);
> +			errtryhelp(EXIT_FAILURE);
>  		}
>  	}
>
> @@ -795,7 +795,7 @@ static void parse_args(int argc, char **argv,
> struct options *op)
>
>  	if (argc < optind + 1) {
>  		log_warn(_("not enough arguments"));
> -		usage(stderr);
> +		errtryhelp(EXIT_FAILURE);
>  	}

BTW here we print the real warning to syslog only. But a few lines below 
we have this
            warn(_("not enough arguments"));
            errtryhelp(EXIT_FAILURE);

Could somebody review this, I'm not sure when agetty is allowed to write 
to the terminal and when not. 

cu,
Rudi

Re: [PATCH 2/4] flock: cleanup usage issues

[Karel Zak]

On Mon, Jun 19, 2017 at 05:45:24PM +0200, Ruediger Meier wrote:
> > > -static void usage(FILE * out) {
> > > +static void usage(void) {
> > > +    FILE * out = stdout;
> > >      fputs(USAGE_HEADER, out);
> > >      ...
> > > }
> >
> > Yes, seems good, but I sure one day someone will ask:
> >
> >     what the hell are you doing with "out = stdout;" ?
> >
> > :-)
> 
> Hehe I know :)
> 
> This is all legacy. It's funny, eventhough I hate usage messages on 
> error, my new checkusage.sh script does it also that way. That's how 
> most simple commands are born ;)

It would be better to make a conclusion (about FILE in the usage())
and follow this conclusion in the patches. I don't think 

  FILE * out = stdout;
  
and replace 'stderr' with 'out' is necessary step. It already changes
all the lines with stderr, so why not use stdout?

If we agree about

  fputs(_("text\n"), stdout);

then we should do the change.

BTW, maybe it would be possible to use Coccinelle to generate the
patches :-)

Comments?

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

Re: [PATCH 4/4] agetty: cleanup usage issues

[Karel Zak]

On Tue, Jun 20, 2017 at 11:35:43AM +0200, Ruediger Meier wrote:
> >  	if (argc < optind + 1) {
> >  		log_warn(_("not enough arguments"));
> > -		usage(stderr);
> > +		errtryhelp(EXIT_FAILURE);
> >  	}
> 
> BTW here we print the real warning to syslog only. But a few lines below 
> we have this
>             warn(_("not enough arguments"));
>             errtryhelp(EXIT_FAILURE);
> 
> Could somebody review this, I'm not sure when agetty is allowed to write 
> to the terminal and when not. 

It's probably bad idea to use stdout/stdin/stderr before open_tty().
The file descriptors maybe open, but for sure it's better to rely on
open_tty().

For parse_args() it would be better to use log_* functions only. It
means that strtou32_or_err() and warnx() are bad idea.

For the --help and --version it's probably fine to use stdout.

    Karel

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

Re: [PATCH 2/4] flock: cleanup usage issues

[Ruediger Meier]

On Tuesday 20 June 2017, Karel Zak wrote:
> On Mon, Jun 19, 2017 at 05:45:24PM +0200, Ruediger Meier wrote:
> > > > -static void usage(FILE * out) {
> > > > +static void usage(void) {
> > > > +    FILE * out = stdout;
> > > >      fputs(USAGE_HEADER, out);
> > > >      ...
> > > > }
> > >
> > > Yes, seems good, but I sure one day someone will ask:
> > >
> > >     what the hell are you doing with "out = stdout;" ?
> > >
> > > :-)
> >
> > Hehe I know :)
> >
> > This is all legacy. It's funny, eventhough I hate usage messages on
> > error, my new checkusage.sh script does it also that way. That's
> > how most simple commands are born ;)
>
> It would be better to make a conclusion (about FILE in the usage())
> and follow this conclusion in the patches. I don't think
>
>   FILE * out = stdout;
>
> and replace 'stderr' with 'out' is necessary step. It already changes
> all the lines with stderr, so why not use stdout?

Well, seems like I've had the talent to find the most unusal 3 files 
first.

You can postpone this pull request. I will send a big one all together,
Seems like I need to push some minor fixes separately before.

> If we agree about
>
>   fputs(_("text\n"), stdout);
>
> then we should do the change.
>
> BTW, maybe it would be possible to use Coccinelle to generate the
> patches :-)
>
> Comments?
>
>     Karel