[PATCH 00/16] pull: changes to various utils

[PATCH 00/16] pull: changes to various utils

[Sami Kerola]

Hello,

Here is collection of patches that hit various utilities all around
source tree, and do not have any particular theme.  These changes are
also available from my github remote repository.

 git://github.com/kerolasa/lelux-utiliteetit.git various

Sami Kerola (16):
  whereis: tell when mandatory option is missing
  flock: add --verbose option
  flock: improve timeout handling
  prlimit: tell in --verbose output which pid got the new limit
  tunelp: remove get_val() in favour of strtol_or_err()
  tunelp: remove unnecessary preprocessor directives
  lib/strutils: move parse_switch() from setterm(1) to library
  tunelp: use parse_switch()
  eject: use parse_switch()
  rpmatch: use symbolic value when evaluation return codes
  tailf: check printing criteria more carefully
  tailf: count last lines correctly at initial print out
  tailf: do not allow minus signed last lines argument
  tailf: ensure file argument really is a file
  logger: move /dev/log to pathnames.h
  logger: fix -i argument parsing regression

 disk-utils/fdisk.c      |   2 +-
 disk-utils/fsck.minix.c |   6 +--
 disk-utils/sfdisk.c     |   2 +-
 include/Makemodule.am   |   1 -
 include/pathnames.h     |   3 ++
 include/rpmatch.h       |   4 ++
 include/strutils.h      |   2 +
 include/timer.h         |  31 -----------
 lib/strutils.c          |   8 +++
 login-utils/vipw.c      |   2 +-
 misc-utils/logger.1     |   5 +-
 misc-utils/logger.c     |  16 ++++--
 misc-utils/whereis.c    |   8 ++-
 sys-utils/Makemodule.am |   5 +-
 sys-utils/eject.c       |  14 +----
 sys-utils/flock.1       |   4 ++
 sys-utils/flock.c       |  82 +++++++++++++++++++++++++----
 sys-utils/prlimit.c     |   3 +-
 sys-utils/tunelp.c      |  51 +++++-------------
 term-utils/mesg.c       |   6 +--
 term-utils/setterm.c    |  14 -----
 text-utils/tailf.c      | 135 ++++++++++++++++++++++++++++--------------------
 22 files changed, 224 insertions(+), 180 deletions(-)
 delete mode 100644 include/timer.h

-- 
2.3.0

[PATCH 01/16] whereis: tell when mandatory option is missing

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/whereis.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/misc-utils/whereis.c b/misc-utils/whereis.c
index 9fc5ee7..c47e122 100644
--- a/misc-utils/whereis.c
+++ b/misc-utils/whereis.c
@@ -495,7 +495,7 @@ int main(int argc, char **argv)
 {
 	struct wh_dirlist *ls = NULL;
 	int want = ALL_DIRS;
-	int i, want_resetable = 0;
+	int i, want_resetable = 0, opt_f_missing = 0;
 
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
@@ -540,6 +540,7 @@ int main(int argc, char **argv)
 
 			switch (*arg) {
 			case 'f':
+				opt_f_missing = 0;
 				break;
 			case 'u':
 				uflag = 1;
@@ -551,6 +552,7 @@ int main(int argc, char **argv)
 				free_dirlist(&ls, BIN_DIR);
 				construct_dirlist_from_argv(
 					&ls, &i, argc, argv, BIN_DIR);
+				opt_f_missing = 1;
 				break;
 			case 'M':
 				if (*(arg + 1))
@@ -559,6 +561,7 @@ int main(int argc, char **argv)
 				free_dirlist(&ls, MAN_DIR);
 				construct_dirlist_from_argv(
 					&ls, &i, argc, argv, MAN_DIR);
+				opt_f_missing = 1;
 				break;
 			case 'S':
 				if (*(arg + 1))
@@ -567,6 +570,7 @@ int main(int argc, char **argv)
 				free_dirlist(&ls, SRC_DIR);
 				construct_dirlist_from_argv(
 					&ls, &i, argc, argv, SRC_DIR);
+				opt_f_missing = 1;
 				break;
 			case 'b':
 				if (want_resetable) {
@@ -607,5 +611,7 @@ int main(int argc, char **argv)
 	}
 
 	free_dirlist(&ls, ALL_DIRS);
+	if (opt_f_missing)
+		errx(EXIT_FAILURE, _("option -f missing"));
 	return EXIT_SUCCESS;
 }
-- 
2.3.0

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Benno Schulenberg]

On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
>  misc-utils/whereis.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

But whereis works fine with a missing -f when some
other option is given after -B or -M or -S.  Example:

$ whereis  -B /crap  -bm  when where whereis
when:
where:
whereis: /usr/share/man/man1/whereis.1.gz

To break that with a complaint about a missing -f...
Better not.

But when there's no intervening option, then it's good
to report a missing -f -- at least, when there is more
than one argument after the -B or -M or -S.

> +	if (opt_f_missing)
> +		errx(EXIT_FAILURE, _("option -f missing"));

s/missing/is missing/

Benno

-- 
http://www.fastmail.com - Send your email first class

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:31PM +0000, Sami Kerola wrote:
>  misc-utils/whereis.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

 Benno is right, see construct_dirlist_from_argv(), the dir list is
 terminated by arbitrary option, it checks for '-'.

 The expected use-case is "<dirlist> <search-restriction> <pattern>",
 for example:

    whereis -M /usr/share/man/man1 -m ls

  Karel

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

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:31PM +0000, Sami Kerola wrote:
> >  misc-utils/whereis.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> 
>  Benno is right, see construct_dirlist_from_argv(), the dir list is
>  terminated by arbitrary option, it checks for '-'.
> 
>  The expected use-case is "<dirlist> <search-restriction> <pattern>",
>  for example:
> 
>     whereis -M /usr/share/man/man1 -m ls

Hi Benno & Karel,

I see, most of the options has to flick the opt_f_missing. Here is updated 
version of the change.

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

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

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Karel Zak]

On Sun, Mar 01, 2015 at 11:08:29AM +0000, Sami Kerola wrote:
> On Tue, 24 Feb 2015, Karel Zak wrote:
> 
> > On Sun, Feb 22, 2015 at 02:41:31PM +0000, Sami Kerola wrote:
> > >  misc-utils/whereis.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> >  Benno is right, see construct_dirlist_from_argv(), the dir list is
> >  terminated by arbitrary option, it checks for '-'.
> > 
> >  The expected use-case is "<dirlist> <search-restriction> <pattern>",
> >  for example:
> > 
> >     whereis -M /usr/share/man/man1 -m ls
> 
> Hi Benno & Karel,
> 
> I see, most of the options has to flick the opt_f_missing. Here is updated 
> version of the change.
> 
> https://github.com/kerolasa/lelux-utiliteetit/commit/ada8f7f0b253a031936519f7953629d701832444

 Did you update your "various" branch? I still see old patches on
 github. (And I'd like to "git pull" :-)

    Karel

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

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Sami Kerola]

On 3 March 2015 at 11:32, Karel Zak <kzak@redhat.com> wrote:
>  Did you update your "various" branch? I still see old patches on
>  github. (And I'd like to "git pull" :-)

I left the original branches as-is. Now when all has had enough time
to have look of my github commits I'll rewrite the history where
needed, and send update pull as various2, script2, more2, and so on
branches. Since that's not a lot of work I assume to do the submission
at evening (work is distracting hobbies).

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

Re: [PATCH 01/16] whereis: tell when mandatory option is missing

[Sami Kerola]

On 3 March 2015 at 12:59, Sami Kerola <kerolasa@iki.fi> wrote:
> On 3 March 2015 at 11:32, Karel Zak <kzak@redhat.com> wrote:
>>  Did you update your "various" branch? I still see old patches on
>>  github. (And I'd like to "git pull" :-)
>
> I left the original branches as-is. Now when all has had enough time
> to have look of my github commits I'll rewrite the history where
> needed, and send update pull as various2, script2, more2, and so on
> branches. Since that's not a lot of work I assume to do the submission
> at evening (work is distracting hobbies).

The second versions of the four branches are available.

Fix to comment Benno mentioned is here. And of course I removed the
earlier mistake addition to unrelated commit.

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

Karel, your proposal to avoid unnecessary fopen() && fclose() pairs
resulted to new commit

https://github.com/kerolasa/lelux-utiliteetit/commit/2a6f4ec60281bf69f1ac2d72d06752c376b78053

that is also below, but does not make the script2 branch quite ready.
The HAVE_LIBUTEMPTER problem is still present.

Summary: the following two branches ought to be ready for merge:
  git://github.com/kerolasa/lelux-utiliteetit.git rtcwake2
  git://github.com/kerolasa/lelux-utiliteetit.git various2

p.s. Here's mail list review copy of commit 2a6f4ec6.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Tue, 3 Mar 2015 22:06:01 +0000
Subject: [PATCH] script: move timing file opening close to use of it

This allows removing almost immediate closure of file handle in the
doshell() function that does not use the file.

Proposed-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/script.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/term-utils/script.c b/term-utils/script.c
index e15ea38..6812548 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -90,6 +90,7 @@ struct script_control {
     char *cflg;        /* command to be executed */
     char *fname;        /* output file path */
     FILE *typescriptfp;    /* output file pointer */
+    char *tname;        /* timing file path */
     FILE *timingfp;        /* timing file pointer */
     struct timeval oldtime;    /* previous write or command start time */
     int master;        /* pseudoterminal master file descriptor */
@@ -169,6 +170,9 @@ static void __attribute__((__noreturn__))
done(struct script_control *ctl)
         else
             exit(WEXITSTATUS(ctl->childstatus));
     }
+    if (ctl->timingfp)
+        fclose(ctl->timingfp);
+    fclose(ctl->typescriptfp);
     exit(EXIT_SUCCESS);
 }

@@ -271,8 +275,17 @@ static void do_io(struct script_control *ctl)
     time_t tvec = time((time_t *)NULL);
     char buf[128];

-    if (ctl->tflg && !ctl->timingfp)
-        ctl->timingfp = fdopen(STDERR_FILENO, "w");
+    if ((ctl->typescriptfp = fopen(ctl->fname, ctl->aflg ? "a" :
"w")) == NULL) {
+        warn(_("cannot open %s"), ctl->fname);
+        fail(ctl);
+    }
+    if (ctl->tflg) {
+        if (!ctl->tname) {
+            if (!(ctl->timingfp = fopen("/dev/stderr", "w")))
+                err(EXIT_FAILURE, _("cannot open %s"), "/dev/stderr");
+        } else if (!(ctl->timingfp = fopen(ctl->tname, "w")))
+            err(EXIT_FAILURE, _("cannot open %s"), ctl->tname);
+    }

     pfd[0].fd = STDIN_FILENO;
     pfd[0].events = POLLIN;
@@ -344,11 +357,6 @@ static void __attribute__((__noreturn__))
doshell(struct script_control *ctl)

     /* close things irrelevant for this process */
     close(ctl->master);
-    if (ctl->typescriptfp)
-        fclose(ctl->typescriptfp);
-    if (ctl->timingfp)
-        fclose(ctl->timingfp);
-    ctl->typescriptfp = ctl->timingfp = NULL;

     dup2(ctl->slave, STDIN_FILENO);
     dup2(ctl->slave, STDOUT_FILENO);
@@ -522,8 +530,8 @@ int main(int argc, char **argv)
             ctl.qflg = 1;
             break;
         case 't':
-            if (optarg && !(ctl.timingfp = fopen(optarg, "w")))
-                err(EXIT_FAILURE, _("cannot open %s"), optarg);
+            if (optarg)
+                ctl.tname = optarg;
             ctl.tflg = 1;
             break;
         case 'V':
@@ -546,11 +554,6 @@ int main(int argc, char **argv)
         die_if_link(&ctl);
     }

-    if ((ctl.typescriptfp = fopen(ctl.fname, ctl.aflg ? "a" : "w")) == NULL) {
-        warn(_("cannot open %s"), ctl.fname);
-        fail(&ctl);
-    }
-
     ctl.shell = getenv("SHELL");
     if (ctl.shell == NULL)
         ctl.shell = _PATH_BSHELL;
-- 
2.3.1

[PATCH 02/16] flock: add --verbose option

[Sami Kerola]

Jenkins script jobs using flock are a great example of a situation in
which one may want an automation to be verbose, so that when unexpected
events happen there is more hints in logs.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/Makemodule.am |  4 ++--
 sys-utils/flock.1       |  4 ++++
 sys-utils/flock.c       | 33 ++++++++++++++++++++++++++++++---
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 15d0a95..d3f77b4 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -1,8 +1,8 @@
 if BUILD_FLOCK
 usrbin_exec_PROGRAMS += flock
 dist_man_MANS += sys-utils/flock.1
-flock_SOURCES = sys-utils/flock.c
-flock_LDADD = $(LDADD) libcommon.la
+flock_SOURCES = sys-utils/flock.c lib/monotonic.c
+flock_LDADD = $(LDADD) libcommon.la $(CLOCKGETTIME_LIBS)
 endif
 
 if BUILD_IPCMK
diff --git a/sys-utils/flock.1 b/sys-utils/flock.1
index cdc294b..f526b33 100644
--- a/sys-utils/flock.1
+++ b/sys-utils/flock.1
@@ -107,6 +107,10 @@ option for the exit code used. The zero number of
 .IR seconds
 is interpreted as \fB\-\-nonblock\fR.
 .TP
+.B \-\-verbose
+Tell why lock could not be got, and when it can be how long did it take
+to acquire the lock.
+.TP
 .BR \-V , " \-\-version"
 Display version information and exit.
 .TP
diff --git a/sys-utils/flock.c b/sys-utils/flock.c
index 3828155..cb8a82b 100644
--- a/sys-utils/flock.c
+++ b/sys-utils/flock.c
@@ -44,6 +44,7 @@
 #include "strutils.h"
 #include "closestream.h"
 #include "timer.h"
+#include "monotonic.h"
 
 static void __attribute__((__noreturn__)) usage(int ex)
 {
@@ -66,6 +67,7 @@ static void __attribute__((__noreturn__)) usage(int ex)
 	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(_(  "     --verbose            increase verbosity\n"), stderr);
 	fprintf(stderr, USAGE_SEPARATOR);
 	fprintf(stderr, USAGE_HELP);
 	fprintf(stderr, USAGE_VERSION);
@@ -120,6 +122,8 @@ int main(int argc, char *argv[])
 	int opt, ix;
 	int do_close = 0;
 	int status;
+	int verbose = 0;
+	struct timeval time_start, time_done;
 	/*
 	 * The default exit code for lock conflict or timeout
 	 * is specified in man flock.1
@@ -128,7 +132,9 @@ int main(int argc, char *argv[])
 	char **cmd_argv = NULL, *sh_c_argv[4];
 	const char *filename = NULL;
 	struct sigaction old_sa;
-
+	enum {
+		OPT_VERBOSE = CHAR_MAX + 1
+	};
 	static const struct option long_options[] = {
 		{"shared", no_argument, NULL, 's'},
 		{"exclusive", no_argument, NULL, 'x'},
@@ -139,8 +145,9 @@ int main(int argc, char *argv[])
 		{"wait", required_argument, NULL, 'w'},
 		{"conflict-exit-code", required_argument, NULL, 'E'},
 		{"close", no_argument, NULL, 'o'},
+		{"verbose", no_argument, NULL, OPT_VERBOSE},
 		{"help", no_argument, NULL, 'h'},
-		{"version", no_argument, NULL, 'V'},
+		{"version", required_argument, NULL, 'V'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -184,6 +191,9 @@ int main(int argc, char *argv[])
 			conflict_exit_code = strtos32_or_err(optarg,
 				_("invalid exit code"));
 			break;
+		case OPT_VERBOSE:
+			verbose = 1;
+			break;
 		case 'V':
 			printf(UTIL_LINUX_VERSION);
 			exit(EX_OK);
@@ -239,16 +249,23 @@ int main(int argc, char *argv[])
 			setup_timer(&timeout, &old_timer, &old_sa, timeout_handler);
 	}
 
+	if (verbose)
+		gettime_monotonic(&time_start);
 	while (flock(fd, type | block)) {
 		switch (errno) {
 		case EWOULDBLOCK:
 			/* -n option set and failed to lock. */
+			if (verbose)
+				warnx(_("failed to get lock"));
 			exit(conflict_exit_code);
 		case EINTR:
 			/* Signal received */
-			if (timeout_expired)
+			if (timeout_expired) {
 				/* -w option set and failed to lock. */
+				if (verbose)
+					warnx(_("timeout while waiting to get lock"));
 				exit(conflict_exit_code);
+			}
 			/* otherwise try again */
 			continue;
 		case EIO:
@@ -282,13 +299,23 @@ int main(int argc, char *argv[])
 
 	if (have_timeout)
 		cancel_timer(&old_timer, &old_sa);
+	if (verbose) {
+		struct timeval delta;
 
+		gettime_monotonic(&time_done);
+		timersub(&time_done, &time_start, &delta);
+		printf(_("%s: getting lock took %ld.%06ld seconds\n"),
+		       program_invocation_short_name, delta.tv_sec,
+		       delta.tv_usec);
+	}
 	status = EX_OK;
 
 	if (cmd_argv) {
 		pid_t w, f;
 		/* Clear any inherited settings */
 		signal(SIGCHLD, SIG_DFL);
+		if (verbose)
+			printf(_("%s: executing %s\n"), program_invocation_short_name, cmd_argv[2]);
 		f = fork();
 
 		if (f < 0) {
-- 
2.3.0

Re: [PATCH 02/16] flock: add --verbose option

[Benno Schulenberg]

On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> +.B \-\-verbose
> +Tell why lock could not be got, and when it can be how long did it take
> +to acquire the lock.

Better: "Report how long it took to acquire the lock, or why the lock
could not be obtained."

> -		{"version", no_argument, NULL, 'V'},
> +		{"version", required_argument, NULL, 'V'},

Huh?

Benno

-- 
http://www.fastmail.com - mmm... Fastmail...

Re: [PATCH 02/16] flock: add --verbose option

[Sami Kerola]

On Sun, 22 Feb 2015, Benno Schulenberg wrote:

> On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> > +.B \-\-verbose
> > +Tell why lock could not be got, and when it can be how long did it take
> > +to acquire the lock.
> 
> Better: "Report how long it took to acquire the lock, or why the lock
> could not be obtained."
> 
> > -		{"version", no_argument, NULL, 'V'},
> > +		{"version", required_argument, NULL, 'V'},
> 
> Huh?

Thanks Benno,

Wording is updated, and --version option argument reuirement reverted. To 
be honest I have no idea how that error slipped to change, excellent you 
noticed it.

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

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

[PATCH 03/16] flock: improve timeout handling

[Sami Kerola]

Signal ALRM raised by the timer, and the timer only, will be considered
as a timeout criteria.

Secondly time interval is made to use monotonic clock.  Documentation of
ITIMER_REAL is unclear whether that time is affected various sources of
clock skew, or does it even tick when system is suspended.

This code is moved from libcommon.la to flock.c because of two reasons.
This is the only utility using the function, and setup_timer() along with
cancel_timer() need to be linked with -lrt option.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/Makemodule.am   |  1 -
 include/timer.h         | 31 -------------------------------
 sys-utils/Makemodule.am |  2 +-
 sys-utils/flock.c       | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 43 insertions(+), 40 deletions(-)
 delete mode 100644 include/timer.h

diff --git a/include/Makemodule.am b/include/Makemodule.am
index c4a52e4..8d7c881 100644
--- a/include/Makemodule.am
+++ b/include/Makemodule.am
@@ -44,7 +44,6 @@ dist_noinst_HEADERS += \
 	include/swapprober.h \
 	include/swapheader.h \
 	include/sysfs.h \
-	include/timer.h \
 	include/timeutils.h \
 	include/ttyutils.h \
 	include/widechar.h \
diff --git a/include/timer.h b/include/timer.h
deleted file mode 100644
index 79ef649..0000000
--- a/include/timer.h
+++ /dev/null
@@ -1,31 +0,0 @@
-#ifndef UTIL_LINUX_TIMER_H
-#define UTIL_LINUX_TIMER_H
-
-#include <signal.h>
-#include <sys/time.h>
-
-static inline int setup_timer(
-			struct itimerval *timer,
-			struct itimerval *old_timer,
-			struct sigaction *old_sa,
-			void (*timeout_handler)(int))
-{
-	struct sigaction sa;
-
-	memset(&sa, 0, sizeof sa);
-	sa.sa_handler = timeout_handler;
-	sa.sa_flags = SA_RESETHAND;
-	sigaction(SIGALRM, &sa, old_sa);
-
-	return setitimer(ITIMER_REAL, timer, old_timer);
-}
-
-static inline void cancel_timer(
-			struct itimerval *old_timer,
-			struct sigaction *old_sa)
-{
-	setitimer(ITIMER_REAL, old_timer, NULL);
-	sigaction(SIGALRM, old_sa, NULL);
-}
-
-#endif
diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index d3f77b4..14d0773 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -2,7 +2,7 @@ if BUILD_FLOCK
 usrbin_exec_PROGRAMS += flock
 dist_man_MANS += sys-utils/flock.1
 flock_SOURCES = sys-utils/flock.c lib/monotonic.c
-flock_LDADD = $(LDADD) libcommon.la $(CLOCKGETTIME_LIBS)
+flock_LDADD = $(LDADD) libcommon.la -lrt
 endif
 
 if BUILD_IPCMK
diff --git a/sys-utils/flock.c b/sys-utils/flock.c
index cb8a82b..97e7325 100644
--- a/sys-utils/flock.c
+++ b/sys-utils/flock.c
@@ -43,7 +43,6 @@
 #include "nls.h"
 #include "strutils.h"
 #include "closestream.h"
-#include "timer.h"
 #include "monotonic.h"
 
 static void __attribute__((__noreturn__)) usage(int ex)
@@ -77,9 +76,12 @@ static void __attribute__((__noreturn__)) usage(int ex)
 
 static sig_atomic_t timeout_expired = 0;
 
-static void timeout_handler(int sig __attribute__((__unused__)))
+static void timeout_handler(int sig __attribute__((__unused__)),
+			    siginfo_t *info,
+			    void *context __attribute__((__unused__)))
 {
-	timeout_expired = 1;
+	if (info->si_code == SI_TIMER)
+		timeout_expired = 1;
 }
 
 static int open_file(const char *filename, int *flags)
@@ -111,9 +113,42 @@ static int open_file(const char *filename, int *flags)
 	return fd;
 }
 
+static int setup_timer(timer_t *t_id, struct itimerval *timeout)
+{
+	struct sigaction sig_a;
+	static struct sigevent sig_e = {
+		.sigev_notify = SIGEV_SIGNAL,
+		.sigev_signo = SIGALRM
+	};
+	struct itimerspec val = {
+		.it_value.tv_sec = timeout->it_value.tv_sec,
+		.it_value.tv_nsec = timeout->it_value.tv_usec * 1000,
+		.it_interval.tv_sec = 0,
+		.it_interval.tv_nsec = 0
+	};
+
+	if (sigemptyset(&sig_a.sa_mask))
+		return 1;
+	sig_a.sa_flags = SA_SIGINFO;
+	sig_a.sa_handler = (void (*)(int))timeout_handler;
+	if (sigaction(SIGALRM, &sig_a, 0))
+		return 1;
+	if (timer_create(CLOCK_MONOTONIC, &sig_e, t_id))
+		return 1;
+	if (timer_settime(*t_id, SA_SIGINFO, &val, NULL))
+		return 1;
+	return 0;
+}
+
+static void cancel_timer(timer_t *t_id)
+{
+	timer_delete(*t_id);
+}
+
 int main(int argc, char *argv[])
 {
-	struct itimerval timeout, old_timer;
+	static timer_t t_id;
+	struct itimerval timeout;
 	int have_timeout = 0;
 	int type = LOCK_EX;
 	int block = 0;
@@ -131,7 +166,6 @@ int main(int argc, char *argv[])
 	int conflict_exit_code = 1;
 	char **cmd_argv = NULL, *sh_c_argv[4];
 	const char *filename = NULL;
-	struct sigaction old_sa;
 	enum {
 		OPT_VERBOSE = CHAR_MAX + 1
 	};
@@ -246,7 +280,8 @@ int main(int argc, char *argv[])
 			have_timeout = 0;
 			block = LOCK_NB;
 		} else
-			setup_timer(&timeout, &old_timer, &old_sa, timeout_handler);
+			if (setup_timer(&t_id, &timeout))
+				err(EX_OSERR, _("cannot not setup timer"));
 	}
 
 	if (verbose)
@@ -298,7 +333,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (have_timeout)
-		cancel_timer(&old_timer, &old_sa);
+		cancel_timer(&t_id);
 	if (verbose) {
 		struct timeval delta;
 
-- 
2.3.0

Re: [PATCH 03/16] flock: improve timeout handling

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:33PM +0000, Sami Kerola wrote:
> Signal ALRM raised by the timer, and the timer only, will be considered
> as a timeout criteria.
> 
> Secondly time interval is made to use monotonic clock.  Documentation of
> ITIMER_REAL is unclear whether that time is affected various sources of
> clock skew, or does it even tick when system is suspended.

I agree, setitimer() API is also obsolete.

> This code is moved from libcommon.la to flock.c because of two reasons.
> This is the only utility using the function, and setup_timer() along with
> cancel_timer() need to be linked with -lrt option.

Hmm... yes, it's used by flock only, but I still think it would be
better to keep it in common lib/ code. Maybe we can use the timers on
another places later (uuidd, login, sulogin, ...).

What about to keep all -lrt stuff in lib/monotonic.c?

> --- a/sys-utils/Makemodule.am
> +++ b/sys-utils/Makemodule.am
> @@ -2,7 +2,7 @@ if BUILD_FLOCK
>  usrbin_exec_PROGRAMS += flock
>  dist_man_MANS += sys-utils/flock.1
>  flock_SOURCES = sys-utils/flock.c lib/monotonic.c
> -flock_LDADD = $(LDADD) libcommon.la $(CLOCKGETTIME_LIBS)
> +flock_LDADD = $(LDADD) libcommon.la -lrt

Don't use -l<foo> in Makefiles, always use $(FOO) and initialize the
variable in ./configure, $(CLOCKGETTIME_LIBS) is fine (although the
name of the variable is not perfect in this context).

> +static void timeout_handler(int sig __attribute__((__unused__)),
> +			    siginfo_t *info,
> +			    void *context __attribute__((__unused__)))
>  {
> -	timeout_expired = 1;
> +	if (info->si_code == SI_TIMER)
> +		timeout_expired = 1;
>  }

BTW, it mean that "kill -ALRM" does not force the program to set 
timeout_expired, right? Nice.

> +static int setup_timer(timer_t *t_id, struct itimerval *timeout)

if you move it to lib/ than add a pointer to timeout_handler() too

Anyway, it seems more elegant than the previous implementation.

    Karel

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

Re: [PATCH 03/16] flock: improve timeout handling

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:33PM +0000, Sami Kerola wrote:
> > Signal ALRM raised by the timer, and the timer only, will be considered
> > as a timeout criteria.
> > 
> > Secondly time interval is made to use monotonic clock.  Documentation of
> > ITIMER_REAL is unclear whether that time is affected various sources of
> > clock skew, or does it even tick when system is suspended.
> 
> I agree, setitimer() API is also obsolete.
> 
> > This code is moved from libcommon.la to flock.c because of two reasons.
> > This is the only utility using the function, and setup_timer() along with
> > cancel_timer() need to be linked with -lrt option.
> 
> Hmm... yes, it's used by flock only, but I still think it would be
> better to keep it in common lib/ code. Maybe we can use the timers on
> another places later (uuidd, login, sulogin, ...).
> 
> What about to keep all -lrt stuff in lib/monotonic.c?

Thanks Karel,

Moving to monotonic sounds good to me. Notice that there is linking change 
in configure.ac making monotonic.c to be linked with -lrt even when 
clock_gettime() would not require it. The timer_create() requires -lrt at 
last for now.

https://github.com/kerolasa/lelux-utiliteetit/commit/50425160a825986abb4e9c22ef8acee8e7b34834

> > --- a/sys-utils/Makemodule.am
> > +++ b/sys-utils/Makemodule.am
> > @@ -2,7 +2,7 @@ if BUILD_FLOCK
> >  usrbin_exec_PROGRAMS += flock
> >  dist_man_MANS += sys-utils/flock.1
> >  flock_SOURCES = sys-utils/flock.c lib/monotonic.c
> > -flock_LDADD = $(LDADD) libcommon.la $(CLOCKGETTIME_LIBS)
> > +flock_LDADD = $(LDADD) libcommon.la -lrt
> 
> Don't use -l<foo> in Makefiles, always use $(FOO) and initialize the
> variable in ./configure, $(CLOCKGETTIME_LIBS) is fine (although the
> name of the variable is not perfect in this context).

TODO:

s/CLOCKGETTIME_LIBS/UL_REALTIME_LIBS/

Better?

> > +static void timeout_handler(int sig __attribute__((__unused__)),
> > +			    siginfo_t *info,
> > +			    void *context __attribute__((__unused__)))
> >  {
> > -	timeout_expired = 1;
> > +	if (info->si_code == SI_TIMER)
> > +		timeout_expired = 1;
> >  }
> 
> BTW, it mean that "kill -ALRM" does not force the program to set 
> timeout_expired, right? Nice.

Correct. After the change flock(1) cannot be forced to timeout by sending 
an ALRM signal. Is that good, bad, or possible regression is debatable. I 
think allowing external signals to cause timeout is unintented behavior. 
If someone requires external timeouts then adding an option to allow them 
is justified.

> > +static int setup_timer(timer_t *t_id, struct itimerval *timeout)
> 
> if you move it to lib/ than add a pointer to timeout_handler() too
> 
> Anyway, it seems more elegant than the previous implementation.

Function pointer is added.

https://github.com/kerolasa/lelux-utiliteetit/commit/50425160a825986abb4e9c22ef8acee8e7b34834

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

Re: [PATCH 03/16] flock: improve timeout handling

[Karel Zak]

On Sun, Mar 01, 2015 at 11:46:11AM +0000, Sami Kerola wrote:
> Moving to monotonic sounds good to me. Notice that there is linking change 
> in configure.ac making monotonic.c to be linked with -lrt even when 
> clock_gettime() would not require it. The timer_create() requires -lrt at 
> last for now.
> 
> https://github.com/kerolasa/lelux-utiliteetit/commit/50425160a825986abb4e9c22ef8acee8e7b34834

OK.

> s/CLOCKGETTIME_LIBS/UL_REALTIME_LIBS/
> 
> Better?

 Yeah.

> Correct. After the change flock(1) cannot be forced to timeout by sending 
> an ALRM signal. Is that good, bad, or possible regression is debatable. I 
> think allowing external signals to cause timeout is unintented behavior. 

 I think it's better to split signals and timers.

> Function pointer is added.
> 
> https://github.com/kerolasa/lelux-utiliteetit/commit/50425160a825986abb4e9c22ef8acee8e7b34834

 You forgot to update commit message ("This code is moved ...").

 Anyway, looks good.

    Karel

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

Re: [PATCH 03/16] flock: improve timeout handling

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:33PM +0000, Sami Kerola wrote:
> +	sig_a.sa_flags = SA_SIGINFO;
> +	sig_a.sa_handler = (void (*)(int))timeout_handler;

man sigaction:

 If SA_SIGINFO is specified in sa_flags, then sa_sigaction (instead of
 sa_handler) specifies the signal-handling function for signum.

I'll fix it...

    Karel

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

[PATCH 04/16] prlimit: tell in --verbose output which pid got the new limit

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/prlimit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sys-utils/prlimit.c b/sys-utils/prlimit.c
index ddab76e..0fa5d57 100644
--- a/sys-utils/prlimit.c
+++ b/sys-utils/prlimit.c
@@ -360,7 +360,8 @@ static void do_prlimit(struct list_head *lims)
 			old = &lim->rlim;
 
 		if (verbose && new) {
-			printf(_("New %s limit: "), lim->desc->name);
+			printf(_("New %s limit for pid %d: "), lim->desc->name,
+				pid ? pid : getpid());
 			if (new->rlim_cur == RLIM_INFINITY)
 				printf("<%s", _("unlimited"));
 			else
-- 
2.3.0

Re: [PATCH 04/16] prlimit: tell in --verbose output which pid got the new limit

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:34PM +0000, Sami Kerola wrote:
>  sys-utils/prlimit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

 Applied, thanks.

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

[PATCH 05/16] tunelp: remove get_val() in favour of strtol_or_err()

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/Makemodule.am |  1 +
 sys-utils/tunelp.c      | 22 ++++++++--------------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 14d0773..c8f1c42 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -50,6 +50,7 @@ if BUILD_TUNELP
 usrsbin_exec_PROGRAMS += tunelp
 dist_man_MANS += sys-utils/tunelp.8
 tunelp_SOURCES = sys-utils/tunelp.c sys-utils/lp.h
+tunelp_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_FSTRIM
diff --git a/sys-utils/tunelp.c b/sys-utils/tunelp.c
index b132d7a..197b093 100644
--- a/sys-utils/tunelp.c
+++ b/sys-utils/tunelp.c
@@ -74,8 +74,10 @@
 #include "xalloc.h"
 #include "closestream.h"
 
-#define EXIT_BAD_VALUE	3
-#define EXIT_LP_IO_ERR	4
+#define STRTOXX_EXIT_CODE	3
+#define EXIT_LP_IO_ERR		4
+
+#include "strutils.h"
 
 struct command {
 	long op;
@@ -114,14 +116,6 @@ static void __attribute__((__noreturn__)) print_usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static long get_val(char *val)
-{
-	long ret;
-	if (!(sscanf(val, "%ld", &ret) == 1))
-		errx(EXIT_BAD_VALUE, _("bad value"));
-	return ret;
-}
-
 static long get_onoff(char *val)
 {
 	if (!strncasecmp("on", val, 2))
@@ -171,28 +165,28 @@ int main(int argc, char **argv)
 			break;
 		case 'i':
 			cmds->op = LPSETIRQ;
-			cmds->val = get_val(optarg);
+			cmds->val = strtol_or_err(optarg, _("argument error"));
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
 		case 't':
 			cmds->op = LPTIME;
-			cmds->val = get_val(optarg);
+			cmds->val = strtol_or_err(optarg, _("argument error"));
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
 		case 'c':
 			cmds->op = LPCHAR;
-			cmds->val = get_val(optarg);
+			cmds->val = strtol_or_err(optarg, _("argument error"));
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
 		case 'w':
 			cmds->op = LPWAIT;
-			cmds->val = get_val(optarg);
+			cmds->val = strtol_or_err(optarg, _("argument error"));
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
-- 
2.3.0

Re: [PATCH 05/16] tunelp: remove get_val() in favour of strtol_or_err()

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:35PM +0000, Sami Kerola wrote:
>  sys-utils/Makemodule.am |  1 +
>  sys-utils/tunelp.c      | 22 ++++++++--------------
>  2 files changed, 9 insertions(+), 14 deletions(-)

 Applied, thanks.

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

[PATCH 06/16] tunelp: remove unnecessary preprocessor directives

[Sami Kerola]

The lp.h included earlier in the tunelp.c has the definitions that were
checked, so these statements could have not been false and such
impossible conditions does not need to be checked.

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

diff --git a/sys-utils/tunelp.c b/sys-utils/tunelp.c
index 197b093..8c70268 100644
--- a/sys-utils/tunelp.c
+++ b/sys-utils/tunelp.c
@@ -205,7 +205,6 @@ int main(int argc, char **argv)
 				show_irq = 0;
 			}
 			break;
-#ifdef LPGETSTATUS
 		case 'o':
 			cmds->op = LPABORTOPEN;
 			cmds->val = get_onoff(optarg);
@@ -228,8 +227,6 @@ int main(int argc, char **argv)
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
-#endif
-#ifdef LPRESET
 		case 'r':
 			cmds->op = LPRESET;
 			cmds->val = 0;
@@ -237,8 +234,6 @@ int main(int argc, char **argv)
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
-#endif
-#ifdef LPTRUSTIRQ
 		case 'T':
 			/* Note: this will do the wrong thing on
 			 * 2.0.36 when compiled under 2.2.x
@@ -249,7 +244,6 @@ int main(int argc, char **argv)
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
-#endif
 		case 'v':
 		case 'V':
 			printf(UTIL_LINUX_VERSION);
@@ -290,7 +284,6 @@ int main(int argc, char **argv)
 
 	cmds = cmdst;
 	while (cmds->next) {
-#ifdef LPGETSTATUS
 		if (cmds->op == LPGETSTATUS) {
 			status = 0xdeadbeef;
 			retval = ioctl(fd, LPGETSTATUS - offset, &status);
@@ -314,7 +307,6 @@ int main(int argc, char **argv)
 				printf("\n");
 			}
 		} else
-#endif /* LPGETSTATUS */
 		if (ioctl(fd, cmds->op - offset, cmds->val) < 0)
 			warn(_("ioctl failed"));
 		cmdst = cmds;
-- 
2.3.0

[PATCH 07/16] lib/strutils: move parse_switch() from setterm(1) to library

[Sami Kerola]

To allow sharing the code with other utilities.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/strutils.h   |  2 ++
 lib/strutils.c       |  8 ++++++++
 term-utils/setterm.c | 14 --------------
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/include/strutils.h b/include/strutils.h
index 4d8463a..ee8c7cb 100644
--- a/include/strutils.h
+++ b/include/strutils.h
@@ -36,6 +36,8 @@ extern void strtotimeval_or_err(const char *str, struct timeval *tv,
 
 extern int isdigit_string(const char *str);
 
+extern int parse_switch(const char *arg, const char *a, const char *b);
+
 #ifndef HAVE_MEMPCPY
 extern void *mempcpy(void *restrict dest, const void *restrict src, size_t n);
 #endif
diff --git a/lib/strutils.c b/lib/strutils.c
index c4f9600..f6154aa 100644
--- a/lib/strutils.c
+++ b/lib/strutils.c
@@ -181,6 +181,14 @@ int isdigit_string(const char *str)
 	return p && p > str && !*p;
 }
 
+int parse_switch(const char *arg, const char *a, const char *b)
+{
+	if (strcmp(arg, a) == 0)
+		return 1;
+	else if (strcmp(arg, b) == 0)
+		return 0;
+	errx(STRTOXX_EXIT_CODE, _("argument error: %s"), arg);
+}
 
 #ifndef HAVE_MEMPCPY
 void *mempcpy(void *restrict dest, const void *restrict src, size_t n)
diff --git a/term-utils/setterm.c b/term-utils/setterm.c
index 76fedba..253f84f 100644
--- a/term-utils/setterm.c
+++ b/term-utils/setterm.c
@@ -178,20 +178,6 @@ struct setterm_control {
 	    opt_powerdown:1, opt_blength:1, opt_bfreq:1;
 };
 
-/* Command line parsing routines.
- *
- * Note that it is an error for a given option to be invoked more than once.
- */
-
-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 int parse_febg_color(const char *arg)
 {
 	int color;
-- 
2.3.0

Re: [PATCH 07/16] lib/strutils: move parse_switch() from setterm(1) to library

[Benno Schulenberg]

On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> -/* Command line parsing routines.
> - *

When removing this comment, please also remove its companion:

/* End of command line parsing routines. */

(It would have been needed anyway, since the usage() routine has
intruded.)

Benno

-- 
http://www.fastmail.com - Access your email from home and the web

Re: [PATCH 07/16] lib/strutils: move parse_switch() from setterm(1) to library

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:37PM +0000, Sami Kerola wrote:
>  include/strutils.h   |  2 ++
>  lib/strutils.c       |  8 ++++++++
>  term-utils/setterm.c | 14 --------------
>  3 files changed, 10 insertions(+), 14 deletions(-)

 Applied (and removed the extra comment as suggested by Benno), thanks.

    Karel

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

[PATCH 08/16] tunelp: use parse_switch()

[Sami Kerola]

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

diff --git a/sys-utils/tunelp.c b/sys-utils/tunelp.c
index 8c70268..65c397a 100644
--- a/sys-utils/tunelp.c
+++ b/sys-utils/tunelp.c
@@ -116,13 +116,6 @@ static void __attribute__((__noreturn__)) print_usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static long get_onoff(char *val)
-{
-	if (!strncasecmp("on", val, 2))
-		return 1;
-	return 0;
-}
-
 int main(int argc, char **argv)
 {
 	int c, fd, irq, status, show_irq, offset = 0, retval;
@@ -193,28 +186,24 @@ int main(int argc, char **argv)
 			break;
 		case 'a':
 			cmds->op = LPABORT;
-			cmds->val = get_onoff(optarg);
+			cmds->val = parse_switch(optarg, "on", "off");
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
 		case 'q':
-			if (get_onoff(optarg)) {
-				show_irq = 1;
-			} else {
-				show_irq = 0;
-			}
+			show_irq = parse_switch(optarg, "on", "off");
 			break;
 		case 'o':
 			cmds->op = LPABORTOPEN;
-			cmds->val = get_onoff(optarg);
+			cmds->val = parse_switch(optarg, "on", "off");
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
 			break;
 		case 'C':
 			cmds->op = LPCAREFUL;
-			cmds->val = get_onoff(optarg);
+			cmds->val = parse_switch(optarg, "on", "off");
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
@@ -239,7 +228,7 @@ int main(int argc, char **argv)
 			 * 2.0.36 when compiled under 2.2.x
 			 */
 			cmds->op = LPTRUSTIRQ;
-			cmds->val = get_onoff(optarg);
+			cmds->val = parse_switch(optarg, "on", "off");
 			cmds->next = xmalloc(sizeof(struct command));
 			cmds = cmds->next;
 			cmds->next = 0;
-- 
2.3.0

Re: [PATCH 08/16] tunelp: use parse_switch()

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:38PM +0000, Sami Kerola wrote:
>  sys-utils/tunelp.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)

 Applied, thanks.

    Karel

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

[PATCH 09/16] eject: use parse_switch()

[Sami Kerola]

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

diff --git a/sys-utils/eject.c b/sys-utils/eject.c
index 4f7d6e6..4e1d65f 100644
--- a/sys-utils/eject.c
+++ b/sys-utils/eject.c
@@ -202,12 +202,7 @@ static void parse_args(struct eject_control *ctl, int argc, char **argv)
 		switch (c) {
 		case 'a':
 			ctl->a_option = 1;
-			if (!strcmp(optarg, "0") || !strcmp(optarg, "off"))
-				ctl->a_arg = 0;
-			else if (!strcmp(optarg, "1") || !strcmp(optarg, "on"))
-				ctl->a_arg = 1;
-			else
-				errx(EXIT_FAILURE, _("invalid argument to --auto/-a option"));
+			ctl->a_arg = parse_switch(optarg, "on", "off");
 			break;
 		case 'c':
 			ctl->c_option = 1;
@@ -231,12 +226,7 @@ static void parse_args(struct eject_control *ctl, int argc, char **argv)
 			break;
 		case 'i':
 			ctl->i_option = 1;
-			if (!strcmp(optarg, "0") || !strcmp(optarg, "off"))
-				ctl->i_arg = 0;
-			else if (!strcmp(optarg, "1") || !strcmp(optarg, "on"))
-				ctl->i_arg = 1;
-			else
-				errx(EXIT_FAILURE, _("invalid argument to --manualeject/-i option"));
+			ctl->i_arg = parse_switch(optarg, "on", "off");
 			break;
 		case 'm':
 			ctl->m_option = 1;
-- 
2.3.0

Re: [PATCH 09/16] eject: use parse_switch()

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:39PM +0000, Sami Kerola wrote:
>  		case 'a':
>  			ctl->a_option = 1;
> -			if (!strcmp(optarg, "0") || !strcmp(optarg, "off"))
> -				ctl->a_arg = 0;
> -			else if (!strcmp(optarg, "1") || !strcmp(optarg, "on"))
> -				ctl->a_arg = 1;
> -			else
> -				errx(EXIT_FAILURE, _("invalid argument to --auto/-a option"));
> +			ctl->a_arg = parse_switch(optarg, "on", "off");

I have extended parse_switch() to accept arbitrary number of 1|0 pairs
and also error message, so:

  ctl->a_arg = parse_switch(optarg, _("argument error"),
                        "on", "off",  "1", "0",  NULL);


Idea: maybe we can remove all the _("argument error") strings from all
strtoxxx_ functions and use some global function to generate proper user 
friendly error messages from longopts[], something like:


while ((c = getopt_long(argc, argv, "a:b:", longopts, NULL)) != -1) {

    err_strtoxxx_set_options(c, longopts);       <--- !!!

    switch (c) {
    case 'a':
        foo = strtol_or_err(optarg);
        break;
    case 'b':
        bar = parse_switch(optarg, "on", "off", NULL);
        break;
    }
}


where err_strtoxxx_set_options() sets in strutils.c static

        ul_cur_option = c;
        ul_lonfopts = longopts;

then on error strtol_or_err() and parse_switch() will use the ul_
variables to print

     "invalid argument to --auto/-a: %s", optarg

and if ul_* are not initialized then something generic ("argument
error"). The result will be less code in main(), better error messages,
and code independent on the names of the options.

    Karel

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

Re: [PATCH 09/16] eject: use parse_switch()

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:39PM +0000, Sami Kerola wrote:
> >  		case 'a':
> >  			ctl->a_option = 1;
> > -			if (!strcmp(optarg, "0") || !strcmp(optarg, "off"))
> > -				ctl->a_arg = 0;
> > -			else if (!strcmp(optarg, "1") || !strcmp(optarg, "on"))
> > -				ctl->a_arg = 1;
> > -			else
> > -				errx(EXIT_FAILURE, _("invalid argument to --auto/-a option"));
> > +			ctl->a_arg = parse_switch(optarg, "on", "off");
> 
> I have extended parse_switch() to accept arbitrary number of 1|0 pairs
> and also error message, so:
> 
>   ctl->a_arg = parse_switch(optarg, _("argument error"),
>                         "on", "off",  "1", "0",  NULL);
> 
> 
> Idea: maybe we can remove all the _("argument error") strings from all
> strtoxxx_ functions and use some global function to generate proper user 
> friendly error messages from longopts[], something like:
> 
> 
> while ((c = getopt_long(argc, argv, "a:b:", longopts, NULL)) != -1) {
> 
>     err_strtoxxx_set_options(c, longopts);       <--- !!!
> 
>     switch (c) {
>     case 'a':
>         foo = strtol_or_err(optarg);
>         break;
>     case 'b':
>         bar = parse_switch(optarg, "on", "off", NULL);
>         break;
>     }
> }
> 
> 
> where err_strtoxxx_set_options() sets in strutils.c static
> 
>         ul_cur_option = c;
>         ul_lonfopts = longopts;
> 
> then on error strtol_or_err() and parse_switch() will use the ul_
> variables to print
> 
>      "invalid argument to --auto/-a: %s", optarg
> 
> and if ul_* are not initialized then something generic ("argument
> error"). The result will be less code in main(), better error messages,
> and code independent on the names of the options.

Thanks Karel,

Updated version of parse_switch() is now in use in 'tunelp: use 
parse_switch()' change.

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

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

[PATCH 10/16] rpmatch: use symbolic value when evaluation return codes

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/fdisk.c      | 2 +-
 disk-utils/fsck.minix.c | 6 +++---
 disk-utils/sfdisk.c     | 2 +-
 include/rpmatch.h       | 4 ++++
 login-utils/vipw.c      | 2 +-
 term-utils/mesg.c       | 6 +++---
 6 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/disk-utils/fdisk.c b/disk-utils/fdisk.c
index 8a0bfd5..815f3f3 100644
--- a/disk-utils/fdisk.c
+++ b/disk-utils/fdisk.c
@@ -347,7 +347,7 @@ int ask_callback(struct fdisk_context *cxt, struct fdisk_ask *ask,
 			if (rc)
 				break;
 			x = rpmatch(buf);
-			if (x == 1 || x == 0) {
+			if (x == RPMATCH_YES || x == RPMATCH_NO) {
 				fdisk_ask_yesno_set_result(ask, x);
 				break;
 			}
diff --git a/disk-utils/fsck.minix.c b/disk-utils/fsck.minix.c
index ac2dc47..52b001b 100644
--- a/disk-utils/fsck.minix.c
+++ b/disk-utils/fsck.minix.c
@@ -269,11 +269,11 @@ ask(const char *string, int def) {
 	ignore_result( fgets(input, YESNO_LENGTH, stdin) );
 	resp = rpmatch(input);
 	switch (resp) {
-	case -1:
+	case RPMATCH_INVALID:
 		/* def = def */
 		break;
-	case 0:
-	case 1:
+	case RPMATCH_NO:
+	case RPMATCH_YES:
 		def = resp;
 		break;
 	default:
diff --git a/disk-utils/sfdisk.c b/disk-utils/sfdisk.c
index b7d4480..bce94e5 100644
--- a/disk-utils/sfdisk.c
+++ b/disk-utils/sfdisk.c
@@ -167,7 +167,7 @@ static int ask_callback(struct fdisk_context *cxt,
 			if (rc)
 				break;
 			x = rpmatch(buf);
-			if (x == 1 || x == 0) {
+			if (x == RPMATCH_YES || x == RPMATCH_NO) {
 				fdisk_ask_yesno_set_result(ask, x);
 				break;
 			}
diff --git a/include/rpmatch.h b/include/rpmatch.h
index d62634b..f64d52e 100644
--- a/include/rpmatch.h
+++ b/include/rpmatch.h
@@ -6,4 +6,8 @@
 	(*r == 'y' || *r == 'Y' ? 1 : *r == 'n' || *r == 'N' ? 0 : -1)
 #endif
 
+#define RPMATCH_YES	 1
+#define RPMATCH_NO	 0
+#define RPMATCH_INVALID	-1
+
 #endif /* UTIL_LINUX_RPMATCH_H */
diff --git a/login-utils/vipw.c b/login-utils/vipw.c
index 668f4d8..9fb2550 100644
--- a/login-utils/vipw.c
+++ b/login-utils/vipw.c
@@ -352,7 +352,7 @@ int main(int argc, char *argv[])
 		printf(_("Would you like to edit %s now [y/n]? "), orig_file);
 
 		if (fgets(response, sizeof(response), stdin)) {
-			if (rpmatch(response) == 1)
+			if (rpmatch(response) == RPMATCH_YES)
 				edit_file(1);
 		}
 	}
diff --git a/term-utils/mesg.c b/term-utils/mesg.c
index 3cb1d24..77d89c7 100644
--- a/term-utils/mesg.c
+++ b/term-utils/mesg.c
@@ -137,7 +137,7 @@ int main(int argc, char *argv[])
 	}
 
 	switch (rpmatch(argv[0])) {
-	case 1:
+	case RPMATCH_YES:
 #ifdef USE_TTY_GROUP
 		if (chmod(tty, sb.st_mode | S_IWGRP) < 0)
 #else
@@ -147,13 +147,13 @@ int main(int argc, char *argv[])
 		if (verbose)
 			puts(_("write access to your terminal is allowed"));
 		return IS_ALLOWED;
-	case 0:
+	case RPMATCH_NO:
 		if (chmod(tty, sb.st_mode & ~(S_IWGRP|S_IWOTH)) < 0)
 			 err(MESG_EXIT_FAILURE, _("change %s mode failed"), tty);
 		if (verbose)
 			puts(_("write access to your terminal is denied"));
 		return IS_NOT_ALLOWED;
-        case -1:
+	case RPMATCH_INVALID:
 		warnx(_("invalid argument: %s"), argv[0]);
 		usage(stderr);
         default:
-- 
2.3.0

Re: [PATCH 10/16] rpmatch: use symbolic value when evaluation return codes

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:40PM +0000, Sami Kerola wrote:
>  disk-utils/fdisk.c      | 2 +-
>  disk-utils/fsck.minix.c | 6 +++---
>  disk-utils/sfdisk.c     | 2 +-
>  include/rpmatch.h       | 4 ++++
>  login-utils/vipw.c      | 2 +-
>  term-utils/mesg.c       | 6 +++---
>  6 files changed, 13 insertions(+), 9 deletions(-)

 Applied, thanks.

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

[PATCH 11/16] tailf: check printing criteria more carefully

[Sami Kerola]

Earlier size check did not print out contents of a file if it happen to
be same size as last time roll_file() function ran.  The earlier buggy
behavior was easy to reproduce with two terminal windows.  Contents of
the second echo were not visible.

term1> tailf /tmp/tailf-test
terminal2> echo 1 >| /tmp/tailf-test
terminal2> echo 2 >| /tmp/tailf-test

In this commit also a mount point and a file changes are detected,
leading to full printout of a file in such cases.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 text-utils/tailf.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/text-utils/tailf.c b/text-utils/tailf.c
index 38df99f..0c477ad 100644
--- a/text-utils/tailf.c
+++ b/text-utils/tailf.c
@@ -86,7 +86,7 @@ tailf(const char *filename, int lines)
 }
 
 static void
-roll_file(const char *filename, off_t *size)
+roll_file(const char *filename, struct stat *old)
 {
 	char buf[BUFSIZ];
 	int fd;
@@ -100,12 +100,17 @@ roll_file(const char *filename, off_t *size)
 	if (fstat(fd, &st) == -1)
 		err(EXIT_FAILURE, _("stat of %s failed"), filename);
 
-	if (st.st_size == *size) {
+	if (st.st_size == old->st_size && st.st_mtime == old->st_mtime &&
+	    st.st_ino == old->st_ino && st.st_dev == old->st_dev) {
 		close(fd);
 		return;
 	}
 
-	if (lseek(fd, *size, SEEK_SET) != (off_t)-1) {
+	if (st.st_ino != old->st_ino || st.st_dev != old->st_dev
+	    || (old->st_mtime < st.st_mtime && st.st_size <= old->st_size))
+		old->st_size = 0;
+
+	if (lseek(fd, old->st_size, SEEK_SET) != (off_t)-1) {
 		ssize_t rc, wc;
 
 		while ((rc = read(fd, buf, sizeof(buf))) > 0) {
@@ -123,16 +128,16 @@ roll_file(const char *filename, off_t *size)
 	 * avoids data duplication. If we read nothing or hit an error, reset
 	 * to the reported size, this handles truncated files.
 	 */
-	*size = (pos != -1 && pos != *size) ? pos : st.st_size;
+	old->st_size = (pos != -1 && pos != old->st_size) ? pos : 0;
 
 	close(fd);
 }
 
 static void
-watch_file(const char *filename, off_t *size)
+watch_file(const char *filename, struct stat *old)
 {
 	do {
-		roll_file(filename, size);
+		roll_file(filename, old);
 		xusleep(250000);
 	} while(1);
 }
@@ -144,7 +149,7 @@ watch_file(const char *filename, off_t *size)
 #define NEVENTS		4
 
 static int
-watch_file_inotify(const char *filename, off_t *size)
+watch_file_inotify(const char *filename, struct stat *old)
 {
 	char buf[ NEVENTS * sizeof(struct inotify_event) ];
 	int fd, ffd, e;
@@ -176,7 +181,7 @@ watch_file_inotify(const char *filename, off_t *size)
 			struct inotify_event *ev = (struct inotify_event *) &buf[e];
 
 			if (ev->mask & IN_MODIFY)
-				roll_file(filename, size);
+				roll_file(filename, old);
 			else {
 				close(ffd);
 				ffd = -1;
@@ -237,8 +242,7 @@ int main(int argc, char **argv)
 	const char *filename;
 	long lines;
 	int ch;
-	struct stat st;
-	off_t size = 0;
+	struct stat old;
 
 	static const struct option longopts[] = {
 		{ "lines",   required_argument, 0, 'n' },
@@ -277,16 +281,15 @@ int main(int argc, char **argv)
 
 	filename = argv[optind];
 
-	if (stat(filename, &st) != 0)
+	if (stat(filename, &old) != 0)
 		err(EXIT_FAILURE, _("stat of %s failed"), filename);
 
-	size = st.st_size;;
 	tailf(filename, lines);
 
 #ifdef HAVE_INOTIFY_INIT
-	if (!watch_file_inotify(filename, &size))
+	if (!watch_file_inotify(filename, &old))
 #endif
-		watch_file(filename, &size);
+		watch_file(filename, &old);
 
 	return EXIT_SUCCESS;
 }
-- 
2.3.0

Re: [PATCH 11/16] tailf: check printing criteria more carefully

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:41PM +0000, Sami Kerola wrote:
> -	if (lseek(fd, *size, SEEK_SET) != (off_t)-1) {
> +	if (st.st_ino != old->st_ino || st.st_dev != old->st_dev
> +	    || (old->st_mtime < st.st_mtime && st.st_size <= old->st_size))
> +		old->st_size = 0;

 So.. if I remove (truncate) last line from 1GiB file then tailf will 
 print all the file *again*? Would be better to reset to st.st_size if the
 size is smaller than old->st_size?

 Or maybe if you want to support "echo 1 >| file" use case than add a
 new option (--rewind) or so.

> +	if (lseek(fd, old->st_size, SEEK_SET) != (off_t)-1) {
>  		ssize_t rc, wc;
>  
>  		while ((rc = read(fd, buf, sizeof(buf))) > 0) {
> @@ -123,16 +128,16 @@ roll_file(const char *filename, off_t *size)
>  	 * avoids data duplication. If we read nothing or hit an error, reset
>  	 * to the reported size, this handles truncated files.
>  	 */
> -	*size = (pos != -1 && pos != *size) ? pos : st.st_size;
> +	old->st_size = (pos != -1 && pos != old->st_size) ? pos : 0;

 This way how to reset seems better.

    Karel


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

Re: [PATCH 11/16] tailf: check printing criteria more carefully

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:41PM +0000, Sami Kerola wrote:
> > -	if (lseek(fd, *size, SEEK_SET) != (off_t)-1) {
> > +	if (st.st_ino != old->st_ino || st.st_dev != old->st_dev
> > +	    || (old->st_mtime < st.st_mtime && st.st_size <= old->st_size))
> > +		old->st_size = 0;
> 
>  So.. if I remove (truncate) last line from 1GiB file then tailf will 
>  print all the file *again*? Would be better to reset to st.st_size if the
>  size is smaller than old->st_size?
> 
>  Or maybe if you want to support "echo 1 >| file" use case than add a
>  new option (--rewind) or so.

Thanks Karel,

The 1GiB file last line truncation is such strong argument against the 
change that I removed it from my remote branch. Maybe the tailf.1 manual 
page should mention truncations are not handled always correctly, and 
that's as good the command is ever going to be.

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

[PATCH 12/16] tailf: count last lines correctly at initial print out

[Sami Kerola]

When last lines happen to be greater than string buffer size for fgets()
the number of printed lines resulted to too few.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 text-utils/tailf.c | 56 +++++++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/text-utils/tailf.c b/text-utils/tailf.c
index 0c477ad..b5ea4c2 100644
--- a/text-utils/tailf.c
+++ b/text-utils/tailf.c
@@ -36,6 +36,8 @@
 #include <ctype.h>
 #include <errno.h>
 #include <getopt.h>
+#include <sys/mman.h>
+
 #ifdef HAVE_INOTIFY_INIT
 #include <sys/inotify.h>
 #endif
@@ -49,40 +51,34 @@
 #define DEFAULT_LINES  10
 
 static void
-tailf(const char *filename, int lines)
+tailf(const char *filename, unsigned long lines, struct stat *old)
 {
-	char *buf, *p;
-	int  head = 0;
-	int  tail = 0;
-	FILE *str;
-	int  i;
+	int fd;
+	size_t i;
+	char *data;
 
-	if (!(str = fopen(filename, "r")))
+	if (!(fd = open(filename, O_RDONLY)))
 		err(EXIT_FAILURE, _("cannot open %s"), filename);
-
-	buf = xmalloc((lines ? lines : 1) * BUFSIZ);
-	p = buf;
-	while (fgets(p, BUFSIZ, str)) {
-		if (++tail >= lines) {
-			tail = 0;
-			head = 1;
+	data = mmap(0, old->st_size, PROT_READ, MAP_SHARED, fd, 0);
+	i = (size_t)old->st_size - 1;
+	/* humans do not think last new line in a file should be counted,
+	 * in that case do off by one from counter point of view */
+	if (data[i] == '\n')
+		lines++;
+	while (i) {
+		if (data[i] == '\n') {
+			if (--lines == 0) {
+				i++;
+				break;
+			}
 		}
-		p = buf + (tail * BUFSIZ);
+		i--;
 	}
-
-	if (head) {
-		for (i = tail; i < lines; i++)
-			fputs(buf + (i * BUFSIZ), stdout);
-		for (i = 0; i < tail; i++)
-			fputs(buf + (i * BUFSIZ), stdout);
-	} else {
-		for (i = head; i < tail; i++)
-			fputs(buf + (i * BUFSIZ), stdout);
-	}
-
+	while (i < (size_t)old->st_size)
+		putchar(data[i++]);
+	munmap(data, old->st_size);
+	close(fd);
 	fflush(stdout);
-	free(buf);
-	fclose(str);
 }
 
 static void
@@ -283,8 +279,8 @@ int main(int argc, char **argv)
 
 	if (stat(filename, &old) != 0)
 		err(EXIT_FAILURE, _("stat of %s failed"), filename);
-
-	tailf(filename, lines);
+	if (lines && old.st_size)
+		tailf(filename, lines, &old);
 
 #ifdef HAVE_INOTIFY_INIT
 	if (!watch_file_inotify(filename, &old))
-- 
2.3.0

Re: [PATCH 12/16] tailf: count last lines correctly at initial print out

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:42PM +0000, Sami Kerola wrote:
> When last lines happen to be greater than string buffer size for fgets()
> the number of printed lines resulted to too few.

 Maybe add a note about mmap() to commit message ?!

> -	if (!(str = fopen(filename, "r")))
> +	if (!(fd = open(filename, O_RDONLY)))
>  		err(EXIT_FAILURE, _("cannot open %s"), filename);
> -
> -	buf = xmalloc((lines ? lines : 1) * BUFSIZ);
> -	p = buf;
> -	while (fgets(p, BUFSIZ, str)) {
> -		if (++tail >= lines) {
> -			tail = 0;
> -			head = 1;
> +	data = mmap(0, old->st_size, PROT_READ, MAP_SHARED, fd, 0);

 It would be nice to compare this it with original implementation on
 large files. I guess mmap will be more effective and faster.

    Karel

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

Re: [PATCH 12/16] tailf: count last lines correctly at initial print out

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:42PM +0000, Sami Kerola wrote:
> > When last lines happen to be greater than string buffer size for fgets()
> > the number of printed lines resulted to too few.
> 
>  Maybe add a note about mmap() to commit message ?!

Thanks Karel,

Done.

https://github.com/kerolasa/lelux-utiliteetit/commit/537f3b197f3c6af307997ebd103becbf44f529c1

> > -	if (!(str = fopen(filename, "r")))
> > +	if (!(fd = open(filename, O_RDONLY)))
> >  		err(EXIT_FAILURE, _("cannot open %s"), filename);
> > -
> > -	buf = xmalloc((lines ? lines : 1) * BUFSIZ);
> > -	p = buf;
> > -	while (fgets(p, BUFSIZ, str)) {
> > -		if (++tail >= lines) {
> > -			tail = 0;
> > -			head = 1;
> > +	data = mmap(0, old->st_size, PROT_READ, MAP_SHARED, fd, 0);
> 
>  It would be nice to compare this it with original implementation on
>  large files. I guess mmap will be more effective and faster.

My motivation to do this change had less to do with speed than correcness. 
Earlier version did not print requested number or lines when tail read 
buffer was insufficient. That probably did not happen too often as 8K is a 
lot of text.

The reason I chose mmap() is simple. It provides easy interface to read a 
file backwards. And a potential speed gain is a nice side effect.

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

[PATCH 13/16] tailf: do not allow minus signed last lines argument

[Sami Kerola]

Before mmap() the command behavior was not completely correct, as
demonstrated below, and after the mmap() it tried to print some eighteen
quintillion lines.

$ tailf -n-1 x
tailf: cannot allocate 18446744073709543424 bytes: Cannot allocate memory

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 text-utils/tailf.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/text-utils/tailf.c b/text-utils/tailf.c
index b5ea4c2..d856203 100644
--- a/text-utils/tailf.c
+++ b/text-utils/tailf.c
@@ -213,16 +213,16 @@ static void __attribute__ ((__noreturn__)) usage(FILE *out)
 }
 
 /* parses -N option */
-static long old_style_option(int *argc, char **argv)
+static long old_style_option(int *argc, char **argv, unsigned long *lines)
 {
-	int i = 1, nargs = *argc;
-	long lines = -1;
+	int i = 1, nargs = *argc, ret = 0;
 
 	while(i < nargs) {
 		if (argv[i][0] == '-' && isdigit(argv[i][1])) {
-			lines = strtol_or_err(argv[i] + 1,
+			*lines = strtoul_or_err(argv[i] + 1,
 					_("failed to parse number of lines"));
 			nargs--;
+			ret = 1;
 			if (nargs - i)
 				memmove(argv + i, argv + i + 1,
 						sizeof(char *) * (nargs - i));
@@ -230,13 +230,13 @@ static long old_style_option(int *argc, char **argv)
 			i++;
 	}
 	*argc = nargs;
-	return lines;
+	return ret;
 }
 
 int main(int argc, char **argv)
 {
 	const char *filename;
-	long lines;
+	unsigned long lines;
 	int ch;
 	struct stat old;
 
@@ -252,16 +252,18 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	lines = old_style_option(&argc, argv);
-	if (lines < 0)
+	if (!old_style_option(&argc, argv, &lines))
 		lines = DEFAULT_LINES;
 
 	while ((ch = getopt_long(argc, argv, "n:N:Vh", longopts, NULL)) != -1)
-		switch((char)ch) {
+		switch ((char)ch) {
 		case 'n':
 		case 'N':
-			lines = strtol_or_err(optarg,
-					_("failed to parse number of lines"));
+			if (optarg[0] == '-')
+				errx(EXIT_FAILURE, "%s: %s",
+				     _("failed to parse number of lines"), optarg);
+			lines =
+			    strtoul_or_err(optarg, _("failed to parse number of lines"));
 			break;
 		case 'V':
 			printf(UTIL_LINUX_VERSION);
-- 
2.3.0

[PATCH 14/16] tailf: ensure file argument really is a file

[Sami Kerola]

The tailf(1) never worked very well with block or character devices,
sockets, fifos and such.  Now after mmap() is used to find last lines
even the little command used to work is broken, so test the tailf is
asked to follow a file and when not fail.  That said symlinks are OK, as
long they point to a file.

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

diff --git a/text-utils/tailf.c b/text-utils/tailf.c
index d856203..7f053a4 100644
--- a/text-utils/tailf.c
+++ b/text-utils/tailf.c
@@ -233,6 +233,28 @@ static long old_style_option(int *argc, char **argv, unsigned long *lines)
 	return ret;
 }
 
+static int is_file(const char *filename, const struct stat sb)
+{
+	switch (sb.st_mode & S_IFMT) {
+	case S_IFREG:
+		return 0;
+	case S_IFLNK:
+		{
+			char *resolved_path = NULL;
+			struct stat follow;
+			int ret;
+
+			if (realpath(filename, resolved_path)) {
+				stat(resolved_path, &follow);
+				ret = is_file(resolved_path, follow);
+				free(resolved_path);
+				return ret;
+			}
+		}
+	}
+	return 1;
+}
+
 int main(int argc, char **argv)
 {
 	const char *filename;
@@ -281,6 +303,8 @@ int main(int argc, char **argv)
 
 	if (stat(filename, &old) != 0)
 		err(EXIT_FAILURE, _("stat of %s failed"), filename);
+	if (is_file(filename, old))
+		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
 	if (lines && old.st_size)
 		tailf(filename, lines, &old);
 
-- 
2.3.0

Re: [PATCH 14/16] tailf: ensure file argument really is a file

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:44PM +0000, Sami Kerola wrote:
>  text-utils/tailf.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/text-utils/tailf.c b/text-utils/tailf.c
> index d856203..7f053a4 100644
> --- a/text-utils/tailf.c
> +++ b/text-utils/tailf.c
> @@ -233,6 +233,28 @@ static long old_style_option(int *argc, char **argv, unsigned long *lines)
>  	return ret;
>  }
>  
> +static int is_file(const char *filename, const struct stat sb)
> +{
> +	switch (sb.st_mode & S_IFMT) {
> +	case S_IFREG:
> +		return 0;
> +	case S_IFLNK:
> +		{
> +			char *resolved_path = NULL;
> +			struct stat follow;
> +			int ret;
> +
> +			if (realpath(filename, resolved_path)) {
> +				stat(resolved_path, &follow);
> +				ret = is_file(resolved_path, follow);
> +				free(resolved_path);
> +				return ret;
> +			}
> +		}

 but you use stat() and no lstat(), so you don't have to care about
 symlinks, right? :-)

>  	if (stat(filename, &old) != 0)
>  		err(EXIT_FAILURE, _("stat of %s failed"), filename);

 if (IS_REG(old.st_mode))
        errx(EXIT_FAILURE, _("%s: is not a file"), filename);

> +	if (is_file(filename, old))
> +		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
>  	if (lines && old.st_size)
>  		tailf(filename, lines, &old);

   Karel


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

Re: [PATCH 14/16] tailf: ensure file argument really is a file

[Sami Kerola]

On Tue, 24 Feb 2015, Karel Zak wrote:

> On Sun, Feb 22, 2015 at 02:41:44PM +0000, Sami Kerola wrote:
> >  text-utils/tailf.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/text-utils/tailf.c b/text-utils/tailf.c
> > index d856203..7f053a4 100644
> > --- a/text-utils/tailf.c
> > +++ b/text-utils/tailf.c
> > @@ -233,6 +233,28 @@ static long old_style_option(int *argc, char **argv, unsigned long *lines)
> >  	return ret;
> >  }
> >  
> > +static int is_file(const char *filename, const struct stat sb)
> > +{
> > +	switch (sb.st_mode & S_IFMT) {
> > +	case S_IFREG:
> > +		return 0;
> > +	case S_IFLNK:
> > +		{
> > +			char *resolved_path = NULL;
> > +			struct stat follow;
> > +			int ret;
> > +
> > +			if (realpath(filename, resolved_path)) {
> > +				stat(resolved_path, &follow);
> > +				ret = is_file(resolved_path, follow);
> > +				free(resolved_path);
> > +				return ret;
> > +			}
> > +		}
> 
>  but you use stat() and no lstat(), so you don't have to care about
>  symlinks, right? :-)
> 
> >  	if (stat(filename, &old) != 0)
> >  		err(EXIT_FAILURE, _("stat of %s failed"), filename);
> 
>  if (IS_REG(old.st_mode))
>         errx(EXIT_FAILURE, _("%s: is not a file"), filename);

Thanks Karel,

Change is made a lot nicer, and I found what got side lined;

((st.st_mode & S_IFMT) == S_IFREG) != S_ISREG(st.st_mode)

Let's hope I will never forget that in future.

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

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

[PATCH 15/16] logger: move /dev/log to pathnames.h

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/pathnames.h | 3 +++
 misc-utils/logger.c | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/pathnames.h b/include/pathnames.h
index cbc93b7..cc01589 100644
--- a/include/pathnames.h
+++ b/include/pathnames.h
@@ -193,5 +193,8 @@
 /* kernel command line */
 #define _PATH_PROC_CMDLINE	"/proc/cmdline"
 
+/* logger paths */
+#define _PATH_DEVLOG		"/dev/log"
+
 #endif /* PATHNAMES_H */
 
diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index a3af7f1..db6fd44 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -56,6 +56,7 @@
 #include "c.h"
 #include "closestream.h"
 #include "nls.h"
+#include "pathnames.h"
 #include "strutils.h"
 #include "xalloc.h"
 
@@ -481,7 +482,7 @@ static void logger_open(struct logger_ctl *ctl)
 			ctl->syslogfp = syslog_rfc5424;
 		return;
 	}
-	ctl->fd = unix_socket(ctl, "/dev/log", ctl->socket_type);
+	ctl->fd = unix_socket(ctl, _PATH_DEVLOG, ctl->socket_type);
 	ctl->syslogfp = syslog_local;
 }
 
-- 
2.3.0

Re: [PATCH 15/16] logger: move /dev/log to pathnames.h

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:45PM +0000, Sami Kerola wrote:
>  include/pathnames.h | 3 +++
>  misc-utils/logger.c | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)

 Applied, thanks.

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

[PATCH 16/16] logger: fix -i argument parsing regression

[Sami Kerola]

With earlier logger it's possible to combine the option -i with other
options, such as -s.  But currently:

$:~> logger -is
logger: failed to parse id: 's'

The changed behaviour breaks existing scripts like dhcpcd-run-hooks from
dhcpcd.

Broken-since: aab5b44405b9a6ada92e419e5a84cc0d1d4afee9
Reference: http://comments.gmane.org/gmane.linux.utilities.util-linux-ng/9683
Reported-by: Juergen Daubert <jue@jue.li>
Reviewed-by: Benno Schulenberg <benno@vertaalt.nl>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/logger.1 |  5 ++++-
 misc-utils/logger.c | 13 +++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/misc-utils/logger.1 b/misc-utils/logger.1
index a855cdf..1bf8f2e 100644
--- a/misc-utils/logger.1
+++ b/misc-utils/logger.1
@@ -58,7 +58,10 @@ syslog port defined in /etc/services, which is often 514 .
 Log the contents of the specified \fIfile\fR.
 This option cannot be combined with a command-line message.
 .TP
-.BR \-i , " \-\-id" [ = \fIid ]
+.BR \-i
+Log the PID of the logger process with each line.
+.TP
+.BR "\-\-id" [ = \fIid ]
 Log the PID of the logger process with each line.  When the optional
 argument \fIid\fR is specified, then it is used instead of the logger
 command's PID.  The use of \fB\-\-id=$$\fR
diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index db6fd44..9f9af7e 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -89,7 +89,8 @@ enum {
 	OPT_JOURNALD,
 	OPT_RFC3164,
 	OPT_RFC5424,
-	OPT_SOCKET_ERRORS
+	OPT_SOCKET_ERRORS,
+	OPT_ID
 };
 
 struct logger_ctl {
@@ -548,7 +549,8 @@ static void __attribute__ ((__noreturn__)) usage(FILE *out)
 	fputs(_("Enter messages into the system log.\n"), out);
 
 	fputs(USAGE_OPTIONS, out);
-	fputs(_(" -i, --id[=<id>]          log <id> (default is PID)\n"), out);
+	fputs(_(" -i                       log logger command PID\n"), out);
+	fputs(_("     --id[=<id>]          log <id> (default is PID)\n"), out);
 	fputs(_(" -f, --file <file>        log the contents of this file\n"), out);
 	fputs(_(" -p, --priority <prio>    mark given message with this priority\n"), out);
 	fputs(_("     --prio-prefix        look for a prefix on every line read from stdin\n"), out);
@@ -606,7 +608,7 @@ int main(int argc, char **argv)
 	FILE *jfd = NULL;
 #endif
 	static const struct option longopts[] = {
-		{ "id",		optional_argument,  0, 'i' },
+		{ "id",		optional_argument,  0, OPT_ID },
 		{ "stderr",	no_argument,	    0, 's' },
 		{ "file",	required_argument,  0, 'f' },
 		{ "priority",	required_argument,  0, 'p' },
@@ -633,7 +635,7 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((ch = getopt_long(argc, argv, "f:i::p:st:u:dTn:P:Vh",
+	while ((ch = getopt_long(argc, argv, "f:ip:st:u:dTn:P:Vh",
 					    longopts, NULL)) != -1) {
 		switch (ch) {
 		case 'f':		/* file to log */
@@ -642,6 +644,9 @@ int main(int argc, char **argv)
 			stdout_reopened = 1;
 			break;
 		case 'i':		/* log process id also */
+			ctl.pid = getpid();
+			break;
+		case OPT_ID:
 			if (optarg) {
 				const char *p = optarg;
 
-- 
2.3.0

Re: [PATCH 16/16] logger: fix -i argument parsing regression

[Benno Schulenberg]

On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> Reviewed-by: Benno Schulenberg <benno@vertaalt.nl>

Please use my justemail address (the current one) here instead.
The vertaalt one I use just for translations.

> +.BR \-i

s/BR/B/    (since there is just one argument)

> +.BR "\-\-id" [ = \fIid ]

s/= /=/    (to fix my own mistake)

> -	fputs(_(" -i, --id[=<id>]          log <id> (default is PID)\n"), out);
> +	fputs(_(" -i                       log logger command PID\n"), out);

Better: "log the logger command's PID" -- there is room enough to
avoid telegram style.

> +	fputs(_("     --id[=<id>]          log <id> (default is PID)\n"), out);

Better: "log the given <id>, or otherwise the PID"

Benno

-- 
http://www.fastmail.com - Access all of your messages and folders
                          wherever you are

Re: [PATCH 16/16] logger: fix -i argument parsing regression

[Sami Kerola]

On Sun, 22 Feb 2015, Benno Schulenberg wrote:

> On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> > Reviewed-by: Benno Schulenberg <benno@vertaalt.nl>
> 
> Please use my justemail address (the current one) here instead.
> The vertaalt one I use just for translations.
> 
> > +.BR \-i
> 
> s/BR/B/    (since there is just one argument)
> 
> > +.BR "\-\-id" [ = \fIid ]
> 
> s/= /=/    (to fix my own mistake)
> 
> > -	fputs(_(" -i, --id[=<id>]          log <id> (default is PID)\n"), out);
> > +	fputs(_(" -i                       log logger command PID\n"), out);
> 
> Better: "log the logger command's PID" -- there is room enough to
> avoid telegram style.

Nalime Benno,

> > +	fputs(_("     --id[=<id>]          log <id> (default is PID)\n"), out);
> 
> Better: "log the given <id>, or otherwise the PID"

Here is updated version of the change.

https://github.com/kerolasa/lelux-utiliteetit/commit/7dbe2082a9d87316d7aed7d1c8a7f94a8a80b1f0

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

Re: [PATCH 11/62] pull: ...

[Benno Schulenberg]

On Sun, Feb 22, 2015, at 15:41, Sami Kerola wrote:
> Here is collection of patches [...]

Oh mayday, mayday, m...  Where's the panic button?

:)

Benno

-- 
http://www.fastmail.com - Send your email first class

Re: [PATCH 00/16] pull: changes to various utils

[Karel Zak]

On Sun, Feb 22, 2015 at 02:41:30PM +0000, Sami Kerola wrote:
>  git://github.com/kerolasa/lelux-utiliteetit.git various

Merged (various2), thanks.

Look forward to see regression tests for flock, logger and whereis.

    Karel

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