[PATCH 0/5] logger: remove use of libc openlog and other small changes

[PATCH 0/5] logger: remove use of libc openlog and other small changes

[Sami Kerola]

Hello,

In commit eea331abd7159432d8f45f978b2fbac19330ce48 I added an option to
use ppid instead of pid in logger message.  That change worked only when
logging remotely, because the libc syslog(3) call does not support ppid
printing.  For a moment I thought it might be possible to change libc to
have such feature, before realizing there are more than one libc
implementation and making all of them work is hopeless.  So I decided to
avoid the syslog(3) all together, and make syslog_local() to be as close
copy of glibc syslog(3) format as possible with support for ppid
printing.

Rest of the changes in this series are not technically that interesting,
and only remaining thing worth to mention is a small write out, requested
by Karel, about logger changes.

http://www.iki.fi/kerolasa/logger.html


The following changes since commit cd8414f7a13aca0b3ea150b473b83f00819b312f:

  cfdisk: move curs_set(1) to ui_end() (2014-08-06 15:39:27 +0200)

are available in the git repository at:

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

for you to fetch changes up to f0757ac355b42baab1e477345690260db651e693:

  docs: mentintion default udp and tcp ports logger is using (2014-08-09 09:13:25 +0100)

----------------------------------------------------------------
Sami Kerola (5):
      logger: allow use of --id=ppid when logging locally
      logger: remove openlog(3) options
      logger: optimize string initializations
      logger: set function arguments read-only when possible
      docs: mentintion default udp and tcp ports logger is using

 misc-utils/logger.1 |  7 +++++
 misc-utils/logger.c | 84 +++++++++++++++++++++++++++++++++--------------------
 2 files changed, 60 insertions(+), 31 deletions(-)

-- 
2.0.4

[PATCH 1/5] logger: allow use of --id=ppid when logging locally

[Sami Kerola]

There is no obvious way to make syslog(3) to print both pid or ppid, so
duplicate the libc syslog() to logger.  Making the ppid printing work
using unix socket has side effect of local becoming capable to use both
rfc format output, which is hopefully seen as good thing.  The
syslog_local() is format wise one-to-one copy with glibc syslog(3)
format.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/logger.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index 7b10df8..d1b93d0 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -418,7 +418,30 @@ static void parse_rfc5424_flags(struct logger_ctl *ctl, char *optarg)
 
 static void syslog_local(struct logger_ctl *ctl, char *msg)
 {
-	syslog(ctl->pri, "%s", msg);
+	char *buf, *tag;
+	char time[32], pid[32];
+	struct timeval tv;
+	struct tm *tm;
+	pid_t process;
+	int len;
+
+	gettimeofday(&tv, NULL);
+	tm = localtime(&tv.tv_sec);
+	strftime(time, sizeof(time), "%h %e %T", tm);
+
+	tag = ctl->tag ? ctl->tag : program_invocation_short_name;
+
+	if ((process = get_process_id(ctl)))
+		snprintf(pid, sizeof(pid), "[%d]", process);
+	else
+		pid[0] = '\0';
+
+	len = xasprintf(&buf, "<%d>%s %s%s: %s", ctl->pri, time, tag, pid, msg);
+	if (write_all(ctl->fd, buf, len) < 0)
+		warn(_("write failed"));
+	if (ctl->logflags & LOG_PERROR)
+		fprintf(stderr, "%s\n", buf);
+	free(buf);
 }
 
 static void logger_open(struct logger_ctl *ctl)
@@ -435,12 +458,7 @@ static void logger_open(struct logger_ctl *ctl)
 			ctl->syslogfp = syslog_rfc5424;
 		return;
 	}
-
-	if (ctl->syslogfp == syslog_rfc5424 || ctl->syslogfp == syslog_rfc3164)
-		errx(EXIT_FAILURE, _("--server or --socket are required to "
-				     "log by --rfc5424 or --rfc3164."));
-
-	openlog(ctl->tag ? ctl->tag : xgetlogin(), ctl->logflags, 0);
+	ctl->fd = unix_socket("/dev/log", ctl->socket_type);
 	ctl->syslogfp = syslog_local;
 }
 
@@ -493,10 +511,8 @@ static void logger_stdin(struct logger_ctl *ctl)
 
 static void logger_close(struct logger_ctl *ctl)
 {
-	if (!ctl->unix_socket && !ctl->server)
-		closelog();
-	else
-		close(ctl->fd);
+	if (close(ctl->fd) != 0)
+		err(EXIT_FAILURE, _("close failed"));
 }
 
 static void __attribute__ ((__noreturn__)) usage(FILE *out)
-- 
2.0.4

[PATCH 2/5] logger: remove openlog(3) options

[Sami Kerola]

One variable less, and more importantly bit operations become unnecessary
in if statements.

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

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index d1b93d0..8ae00b2 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -82,7 +82,6 @@ enum {
 
 struct logger_ctl {
 	int fd;
-	int logflags;
 	int pri;
 	char *tag;
 	char *unix_socket;
@@ -92,7 +91,9 @@ struct logger_ctl {
 	void (*syslogfp)(struct logger_ctl *ctl, char *msg);
 	unsigned int
 			prio_prefix:1,	/* read priority from intput */
+			pid:1,		/* print PID, or PPID if it is enabled as well*/
 			ppid:1,		/* include PPID instead of PID */
+			stderr_printout:1, /* output message to stderr */
 			rfc5424_time:1,	/* include time stamp */
 			rfc5424_tq:1,	/* include time quality markup */
 			rfc5424_host:1;	/* include hostname */
@@ -291,7 +292,7 @@ static pid_t get_process_id(struct logger_ctl *ctl)
 {
 	pid_t id = 0;
 
-	if (ctl->logflags & LOG_PID)
+	if (ctl->pid)
 		id = ctl->ppid ? getppid() : getpid();
 	return id;
 }
@@ -323,7 +324,7 @@ static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
 
 	if (write_all(ctl->fd, buf, len) < 0)
 		warn(_("write failed"));
-	if (ctl->logflags & LOG_PERROR)
+	if (ctl->stderr_printout)
 		fprintf(stderr, "%s\n", buf);
 
 	free(hostname);
@@ -390,7 +391,7 @@ static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
 	if (write_all(ctl->fd, buf, len) < 0)
 		warn(_("write failed"));
 
-	if (ctl->logflags & LOG_PERROR)
+	if (ctl->stderr_printout)
 		fprintf(stderr, "%s\n", buf);
 
 	free(hostname);
@@ -439,7 +440,7 @@ static void syslog_local(struct logger_ctl *ctl, char *msg)
 	len = xasprintf(&buf, "<%d>%s %s%s: %s", ctl->pri, time, tag, pid, msg);
 	if (write_all(ctl->fd, buf, len) < 0)
 		warn(_("write failed"));
-	if (ctl->logflags & LOG_PERROR)
+	if (ctl->stderr_printout)
 		fprintf(stderr, "%s\n", buf);
 	free(buf);
 }
@@ -557,7 +558,6 @@ int main(int argc, char **argv)
 {
 	struct logger_ctl ctl = {
 		.fd = -1,
-		.logflags = 0,
 		.ppid = 0,
 		.pri = LOG_NOTICE,
 		.prio_prefix = 0,
@@ -611,7 +611,7 @@ int main(int argc, char **argv)
 			stdout_reopened = 1;
 			break;
 		case 'i':		/* log process id also */
-			ctl.logflags |= LOG_PID;
+			ctl.pid = 1;
 			if (optarg) {
 				const char *p = optarg;
 
@@ -629,7 +629,7 @@ int main(int argc, char **argv)
 			ctl.pri = pencode(optarg);
 			break;
 		case 's':		/* log to standard error */
-			ctl.logflags |= LOG_PERROR;
+			ctl.stderr_printout = 1;
 			break;
 		case 't':		/* tag */
 			ctl.tag = optarg;
-- 
2.0.4

[PATCH 3/5] logger: optimize string initializations

[Sami Kerola]

Setting whole array to be completely full of nulls cannot be as quick as
making the only significant member of the array null only when needed.

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

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index 8ae00b2..85f9c35 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -299,7 +299,7 @@ static pid_t get_process_id(struct logger_ctl *ctl)
 
 static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
 {
-	char *buf, pid[30] = { '\0' }, *cp, *tp, *hostname, *dot;
+	char *buf, pid[30], *cp, *tp, *hostname, *dot;
 	time_t now;
 	pid_t process;
 	int len;
@@ -308,6 +308,8 @@ static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
 		return;
 	if ((process = get_process_id(ctl)))
 		snprintf(pid, sizeof(pid), "[%d]", process);
+	else
+		pid[0] = '\0';
 
 	cp = ctl->tag ? ctl->tag : xgetlogin();
 
@@ -334,7 +336,7 @@ static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
 static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
 {
 	char *buf, *tag = NULL, *hostname = NULL;
-	char pid[32] = { '\0' }, time[64] = { '\0' }, timeq[80] = { '\0' };
+	char pid[32], time[64], timeq[80];
 	struct ntptimeval ntptv;
 	struct timeval tv;
 	struct tm *tm;
@@ -354,7 +356,8 @@ static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
 			snprintf(time, sizeof(time), fmt, tv.tv_usec);
 		} else
 			err(EXIT_FAILURE, _("localtime() failed"));
-	}
+	} else
+		time[0] = '\0';
 
 	if (ctl->rfc5424_host) {
 		hostname = xgethostname();
@@ -372,6 +375,8 @@ static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
 
 	if ((process = get_process_id(ctl)))
 		snprintf(pid, sizeof(pid), " %d", process);
+	else
+		pid[0] = '\0';
 
 	if (ctl->rfc5424_tq) {
 		if (ntp_gettime(&ntptv) == TIME_OK)
@@ -381,7 +386,8 @@ static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
 		else
 			snprintf(timeq, sizeof(timeq),
 				 " [timeQuality tzKnown=\"1\" isSynced=\"0\"]");
-	}
+	} else
+		timeq[0] = '\0';
 
 	len = xasprintf(&buf, "<%d>1%s%s%s %s -%s%s %s", ctl->pri, time,
 		  hostname ? " " : "",
-- 
2.0.4

Re: [PATCH 3/5] logger: optimize string initializations

[Karel Zak]

On Sun, Aug 10, 2014 at 12:56:09PM +0100, Sami Kerola wrote:
> -	char pid[32] = { '\0' }, time[64] = { '\0' }, timeq[80] = { '\0' };
> +	char pid[32], time[64], timeq[80];

 I agree that the original code is not elegant, but "else foo[0] =
 '\0'" is also pretty ugly. It would be better to set all to the
 default by one line

   *pid = *time = *timeq = '\0';

 at begin of the function.


>  	if ((process = get_process_id(ctl)))
>  		snprintf(pid, sizeof(pid), " %d", process);
> +	else
> +		pid[0] = '\0';

 I really don't like hairy code like this.

    Karel


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

Re: [PATCH 3/5] logger: optimize string initializations

[Sami Kerola]

On 11 August 2014 14:12, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Aug 10, 2014 at 12:56:09PM +0100, Sami Kerola wrote:
>> -     char pid[32] = { '\0' }, time[64] = { '\0' }, timeq[80] = { '\0' };
>> +     char pid[32], time[64], timeq[80];
>
>  I agree that the original code is not elegant, but "else foo[0] =
>  '\0'" is also pretty ugly. It would be better to set all to the
>  default by one line
>
>    *pid = *time = *timeq = '\0';
>
>  at begin of the function.

Hi Karel,

Done in proposed way.

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

>>       if ((process = get_process_id(ctl)))
>>               snprintf(pid, sizeof(pid), " %d", process);
>> +     else
>> +             pid[0] = '\0';
>
>  I really don't like hairy code like this.

OK, I'll try to be less hairy in future.  We'll see how that goes.
Eventually there will be occasional hairy patch, and hopefully they
are rejected in good will.

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

[PATCH 4/5] logger: set function arguments read-only when possible

[Sami Kerola]

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

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index 85f9c35..8ccc731 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -88,7 +88,7 @@ struct logger_ctl {
 	char *server;
 	char *port;
 	int socket_type;
-	void (*syslogfp)(struct logger_ctl *ctl, char *msg);
+	void (*syslogfp)(const struct logger_ctl *ctl, const char *msg);
 	unsigned int
 			prio_prefix:1,	/* read priority from intput */
 			pid:1,		/* print PID, or PPID if it is enabled as well*/
@@ -118,7 +118,7 @@ static char *get_prio_prefix(char *msg, int *prio)
 	return end + 1;
 }
 
-static int decode(char *name, CODE *codetab)
+static int decode(const char *name, CODE *codetab)
 {
 	register CODE *c;
 
@@ -278,7 +278,7 @@ static int journald_entry(FILE *fp)
 }
 #endif
 
-static char *xgetlogin()
+static char *xgetlogin(void)
 {
 	char *cp;
 	struct passwd *pw;
@@ -288,7 +288,7 @@ static char *xgetlogin()
 	return cp;
 }
 
-static pid_t get_process_id(struct logger_ctl *ctl)
+static pid_t get_process_id(const struct logger_ctl *ctl)
 {
 	pid_t id = 0;
 
@@ -297,7 +297,7 @@ static pid_t get_process_id(struct logger_ctl *ctl)
 	return id;
 }
 
-static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
+static void syslog_rfc3164(const struct logger_ctl *ctl, const char *msg)
 {
 	char *buf, pid[30], *cp, *tp, *hostname, *dot;
 	time_t now;
@@ -333,7 +333,7 @@ static void syslog_rfc3164(struct logger_ctl *ctl, char *msg)
 	free(buf);
 }
 
-static void syslog_rfc5424(struct logger_ctl *ctl, char *msg)
+static void syslog_rfc5424(const  struct logger_ctl *ctl, const char *msg)
 {
 	char *buf, *tag = NULL, *hostname = NULL;
 	char pid[32], time[64], timeq[80];
@@ -423,7 +423,7 @@ static void parse_rfc5424_flags(struct logger_ctl *ctl, char *optarg)
 	}
 }
 
-static void syslog_local(struct logger_ctl *ctl, char *msg)
+static void syslog_local(const struct logger_ctl *ctl, const char *msg)
 {
 	char *buf, *tag;
 	char time[32], pid[32];
@@ -469,7 +469,7 @@ static void logger_open(struct logger_ctl *ctl)
 	ctl->syslogfp = syslog_local;
 }
 
-static void logger_command_line(struct logger_ctl *ctl, char **argv)
+static void logger_command_line(const struct logger_ctl *ctl, char **argv)
 {
 	char buf[4096];
 	char *p = buf;
@@ -516,7 +516,7 @@ static void logger_stdin(struct logger_ctl *ctl)
 	}
 }
 
-static void logger_close(struct logger_ctl *ctl)
+static void logger_close(const struct logger_ctl *ctl)
 {
 	if (close(ctl->fd) != 0)
 		err(EXIT_FAILURE, _("close failed"));
-- 
2.0.4

[PATCH 5/5] docs: mentintion default udp and tcp ports logger is using

[Sami Kerola]

The fact that 'syslog tcp/514' does not exist in RFS's, which has lead
to 'syslog-conn 601/tcp' be used in place could be a suprice and should
be told in manual.

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

diff --git a/misc-utils/logger.1 b/misc-utils/logger.1
index 1a366db..887fff2 100644
--- a/misc-utils/logger.1
+++ b/misc-utils/logger.1
@@ -77,6 +77,13 @@ but if thist fails a TCP connection is attempted.
 \fB\-P\fR, \fB\-\-port\fR \fIport\fR
 Use the specified
 .IR port .
+When
+.I port
+is not specified it defaults to
+.I syslog
+for udp and
+.I syslog-conn
+for tcp connections.
 .TP
 \fB\-f\fR, \fB\-\-file\fR \fIfile\fR
 Log the contents of the specified
-- 
2.0.4

Re: [PATCH 0/5] logger: remove use of libc openlog and other small changes

[Karel Zak]

On Sun, Aug 10, 2014 at 12:56:06PM +0100, Sami Kerola wrote:
> In commit eea331abd7159432d8f45f978b2fbac19330ce48 I added an option to
> use ppid instead of pid in logger message.  That change worked only when

 Maybe it would be better to use --id[=<id>] rather than support PID and 
 PPID only. It means allow to specify arbitrary id, because in some
 more complex situations PPID does not have to be enough.

    Karel

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

Re: [PATCH 0/5] logger: remove use of libc openlog and other small changes

[Karel Zak]

On Mon, Aug 11, 2014 at 03:03:44PM +0200, Karel Zak wrote:
> On Sun, Aug 10, 2014 at 12:56:06PM +0100, Sami Kerola wrote:
> > In commit eea331abd7159432d8f45f978b2fbac19330ce48 I added an option to
> > use ppid instead of pid in logger message.  That change worked only when
> 
>  Maybe it would be better to use --id[=<id>] rather than support PID and 
>  PPID only. It means allow to specify arbitrary id, because in some
>  more complex situations PPID does not have to be enough.

 Implemented, all the logger changes pushed.

    Karel

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