dhcpcd-discuss

Re: dhcpcd logging subsystem modification

Sergey Nikiforov

Tue Feb 11 09:17:57 2020

Hi Roy

On 10.02.2020 18:12, Roy Marples wrote:
Hi Sergey

On 10/02/2020 13:46, Sergey Nikiforov wrote:
PoC patch is attached (not so small because dhcp.c required a lot of changes). It is not complete and is not compilable, I have implemented only "loginfox" and fixed only dhcp.c and ipv6.c.

+/* It is a macro to prevent taking address of it so __FILE__/__LINE__ etc
+ * can easily be added */
+#define loginfox(fmt, ...) loginfox_func(fmt, ##__VA_ARGS__)

##__VA_ARGS__ is gcc specific
__VA_OPT__ is standards compliant

+#define loginfox(fmt, ...) loginfox_func(fmt __VA_OPT__(,) __VA_ARGS__)
But only gcc-8 and clang-6 support that.
It was only 5 days ago where a user supplied a patch for building on old systems without O_CLOEXEC and SOCK_CLOEXEC defined!

However, we can workaround it like so:
+#define loginfox(fmt, ...) loginfox_func(__VA_ARGS__)

This forces the format string into va_args. Luckily in all cases the format string IS the last named parameter so this should be safe. As __VA_ARGS__ is in c99 and we require c99 this should be fine. Might be worth adding a note about that in logerr.h to explain it incase we forget :)


Done. Full implementation is attached. I have tested a build patched for KasperskyOS.


Lastly, I dislike just adding _func to the function name. Just separating with an underscore would be better I think:
+#define loginfox(fmt, ...) log_infox(__VA_ARGS__)

You mean "#define loginfox(...) log_infox(__VA_ARGS__)".

Done.


> BTW code w/o indirect function calls is faster, especially if retpolines are used..

Speed isn't really a concern.
The only concern I have left is that we then need to expose syslog.h and one of the goals of logerr was to hide that away.
Still I don't see anything cleaner just yet.

I do have another question though .... how would you then hook into this?
At a guess you then have your own stubs which take __FILE__ and __LINE__, prepend that into and then call out to the actual functions or another?

#ifdef __KOS__

#define logmessageex(filename, lineno, level, fmt, ...) \
     do { \
         if (level <= LOG_VERBOSITY) { \
            KosLogf(level, filename, lineno, "DHCPCD", fmt, ##__VA_ARGS__); \
         } \
     } while (0)

#define logmessage(level, fmt, ...) \
    logmessageex(__FILE__, __LINE__, level, fmt, ##__VA_ARGS__)
#define logerrmessage(level, fmt, ...) \
    logerrmessageex(__FILE__, __LINE__, level, fmt, ##__VA_ARGS__)
#define logdebug(fmt, ...) logerrmessage(LOG_DEBUG, fmt, ##__VA_ARGS__)
#define logdebugx(fmt, ...) logmessage(LOG_DEBUG, fmt, ##__VA_ARGS__)
#define loginfo(fmt, ...) logerrmessage(LOG_INFO, fmt, ##__VA_ARGS__)
#define loginfox(fmt, ...) logmessage(LOG_INFO, fmt, ##__VA_ARGS__)
#define logwarn(fmt, ...) logerrmessage(LOG_WARN, fmt, ##__VA_ARGS__)
#define logwarnx(fmt, ...) logmessage(LOG_WARN, fmt, ##__VA_ARGS__)
#define logerr(fmt, ...) logerrmessage(LOG_ERR, fmt, ##__VA_ARGS__)
#define logerrx(fmt, ...) logmessage(LOG_ERR, fmt, ##__VA_ARGS__)

#else /* __KOS__ */

...


(I was explicitly asked to use ##__VA_ARGS__ in KasperskyOS to keep "fmt")

logerrmessageex() is slightly more complex but you get the idea.

Thank you.


WBR,

Sergey Nikiforov, Kaspersky Developer


diff --git a/src/dhcp.c b/src/dhcp.c
index 2fdfd837..11a2c18e 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -56,6 +56,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <syslog.h>
 
 #define ELOOP_QUEUE	ELOOP_DHCP
 #include "config.h"
@@ -2818,7 +2819,7 @@ whitelisted_ip(const struct if_options *ifo, in_addr_t addr)
 }
 
 static void
-log_dhcp(logfunc_t *logfunc, const char *msg,
+log_dhcp(int loglevel, const char *msg,
     const struct interface *ifp, const struct bootp *bootp, size_t bootp_len,
     const struct in_addr *from, int ad)
 {
@@ -2865,10 +2866,10 @@ log_dhcp(logfunc_t *logfunc, const char *msg,
 		print_string(sname, sizeof(sname), OT_STRING | OT_DOMAIN,
 		    bootp->sname, sizeof(bootp->sname));
 		if (a == NULL)
-			logfunc("%s: %s %s %s `%s'",
+			logmessage(loglevel, "%s: %s %s %s `%s'",
 			    ifp->name, msg, tfrom, inet_ntoa(addr), sname);
 		else
-			logfunc("%s: %s %s %s %s `%s'",
+			logmessage(loglevel, "%s: %s %s %s %s `%s'",
 			    ifp->name, msg, a, tfrom, inet_ntoa(addr), sname);
 	} else {
 		if (r != 0) {
@@ -2876,10 +2877,10 @@ log_dhcp(logfunc_t *logfunc, const char *msg,
 			addr = *from;
 		}
 		if (a == NULL)
-			logfunc("%s: %s %s %s",
+			logmessage(loglevel, "%s: %s %s %s",
 			    ifp->name, msg, tfrom, inet_ntoa(addr));
 		else
-			logfunc("%s: %s %s %s %s",
+			logmessage(loglevel, "%s: %s %s %s %s",
 			    ifp->name, msg, a, tfrom, inet_ntoa(addr));
 	}
 	free(a);
@@ -3011,7 +3012,7 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 		    (uint8_t *)bootp, bootp_len, 4, type,
 		    auth, auth_len) == NULL)
 		{
-			LOGDHCP0(logerrx, "authentication failed");
+			LOGDHCP0(LOG_ERR, "authentication failed");
 			return;
 		}
 		if (state->auth.token)
@@ -3021,10 +3022,10 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 			loginfox("%s: accepted reconfigure key", ifp->name);
 	} else if (ifo->auth.options & DHCPCD_AUTH_SEND) {
 		if (ifo->auth.options & DHCPCD_AUTH_REQUIRE) {
-			LOGDHCP0(logerrx, "no authentication");
+			LOGDHCP0(LOG_ERR, "no authentication");
 			return;
 		}
-		LOGDHCP0(logwarnx, "no authentication");
+		LOGDHCP0(LOG_WARNING, "no authentication");
 	}
 #endif
 
@@ -3033,20 +3034,20 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 		if (from->s_addr == INADDR_ANY ||
 		    from->s_addr == INADDR_BROADCAST)
 		{
-			LOGDHCP(logerrx, "discarding Force Renew");
+			LOGDHCP(LOG_ERR, "discarding Force Renew");
 			return;
 		}
 #ifdef AUTH
 		if (auth == NULL) {
-			LOGDHCP(logerrx, "unauthenticated Force Renew");
+			LOGDHCP(LOG_ERR, "unauthenticated Force Renew");
 			if (ifo->auth.options & DHCPCD_AUTH_REQUIRE)
 				return;
 		}
 		if (state->state != DHS_BOUND && state->state != DHS_INFORM) {
-			LOGDHCP(logdebugx, "not bound, ignoring Force Renew");
+			LOGDHCP(LOG_DEBUG, "not bound, ignoring Force Renew");
 			return;
 		}
-		LOGDHCP(loginfox, "Force Renew from");
+		LOGDHCP(LOG_INFO, "Force Renew from");
 		/* The rebind and expire timings are still the same, we just
 		 * enter the renew state early */
 		if (state->state == DHS_BOUND)
@@ -3057,19 +3058,19 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 			dhcp_inform(ifp);
 		}
 #else
-		LOGDHCP(logerrx, "unauthenticated Force Renew");
+		LOGDHCP(LOG_ERR, "unauthenticated Force Renew");
 #endif
 		return;
 	}
 
 	if (state->state == DHS_BOUND) {
-		LOGDHCP(logdebugx, "bound, ignoring");
+		LOGDHCP(LOG_DEBUG, "bound, ignoring");
 		return;
 	}
 
 	if (state->state == DHS_PROBE) {
 		/* Ignore any DHCP messages whilst probing a lease to bind. */
-		LOGDHCP(logdebugx, "probing, ignoring");
+		LOGDHCP(LOG_DEBUG, "probing, ignoring");
 		return;
 	}
 
@@ -3082,7 +3083,7 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 		    get_option_uint8(ifp->ctx, &tmp,
 		    bootp, bootp_len, (uint8_t)i) == 0)
 		{
-			LOGDHCP(logwarnx, "reject DHCP");
+			LOGDHCP(LOG_WARNING, "reject DHCP");
 			return;
 		}
 	}
@@ -3093,12 +3094,12 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 		    get_option_addr(ifp->ctx, &addr,
 		    bootp, bootp_len, DHO_SERVERID) == -1)
 		{
-			LOGDHCP(logwarnx, "reject NAK");
+			LOGDHCP(LOG_WARNING, "reject NAK");
 			return;
 		}
 
 		/* We should restart on a NAK */
-		LOGDHCP(logwarnx, "NAK:");
+		LOGDHCP(LOG_WARNING, "NAK:");
 		if ((msg = get_option_string(ifp->ctx,
 		    bootp, bootp_len, DHO_MESSAGE)))
 		{
@@ -3138,14 +3139,14 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 			 * always true. */
 			if (type == 0 && i == DHO_SERVERID)
 				continue;
-			LOGDHCP(logwarnx, "reject DHCP");
+			LOGDHCP(LOG_WARNING, "reject DHCP");
 			return;
 		}
 	}
 
 	/* DHCP Auto-Configure, RFC 2563 */
 	if (type == DHCP_OFFER && bootp->yiaddr == 0) {
-		LOGDHCP(logwarnx, "no address given");
+		LOGDHCP(LOG_WARNING, "no address given");
 		if ((msg = get_option_string(ifp->ctx,
 		    bootp, bootp_len, DHO_MESSAGE)))
 		{
@@ -3159,14 +3160,14 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 		{
 			switch (tmp) {
 			case 0:
-				LOGDHCP(logwarnx, "IPv4LL disabled from");
+				LOGDHCP(LOG_WARNING, "IPv4LL disabled from");
 				ipv4ll_drop(ifp);
 #ifdef ARP
 				arp_drop(ifp);
 #endif
 				break;
 			case 1:
-				LOGDHCP(logwarnx, "IPv4LL enabled from");
+				LOGDHCP(LOG_WARNING, "IPv4LL enabled from");
 				ipv4ll_start(ifp);
 				break;
 			default:
@@ -3189,14 +3190,14 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 	    &&
 	    (bootp->yiaddr == INADDR_ANY || bootp->yiaddr == INADDR_BROADCAST))
 	{
-		LOGDHCP(logwarnx, "reject invalid address");
+		LOGDHCP(LOG_WARNING, "reject invalid address");
 		return;
 	}
 
 #ifdef IN_IFF_DUPLICATED
 	ia = ipv4_iffindaddr(ifp, &lease->addr, NULL);
 	if (ia && ia->addr_flags & IN_IFF_DUPLICATED) {
-		LOGDHCP(logwarnx, "declined duplicate address");
+		LOGDHCP(LOG_WARNING, "declined duplicate address");
 		if (type)
 			dhcp_decline(ifp);
 		ipv4_deladdr(ia, 0);
@@ -3227,7 +3228,7 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 			goto rapidcommit;
 		}
 
-		LOGDHCP(loginfox, "offered");
+		LOGDHCP(LOG_INFO, "offered");
 		if (state->offer_len < bootp_len) {
 			free(state->offer);
 			if ((state->offer = malloc(bootp_len)) == NULL) {
@@ -3269,13 +3270,13 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 
 	if (type) {
 		if (type == DHCP_OFFER) {
-			LOGDHCP(logwarnx, "ignoring offer of");
+			LOGDHCP(LOG_WARNING, "ignoring offer of");
 			return;
 		}
 
 		/* We should only be dealing with acks */
 		if (type != DHCP_ACK) {
-			LOGDHCP(logerr, "not ACK or OFFER");
+			LOGDHCP(LOG_ERR, "not ACK or OFFER");
 			return;
 		}
 
@@ -3287,14 +3288,14 @@ dhcp_handledhcp(struct interface *ifp, struct bootp *bootp, size_t bootp_len,
 			    DHO_RAPIDCOMMIT, NULL))
 				state->state = DHS_REQUEST;
 			else {
-				LOGDHCP(logdebugx, "ignoring ack of");
+				LOGDHCP(LOG_DEBUG, "ignoring ack of");
 				return;
 			}
 		}
 
 rapidcommit:
 		if (!(ifo->options & DHCPCD_INFORM))
-			LOGDHCP(logdebugx, "acknowledged");
+			LOGDHCP(LOG_DEBUG, "acknowledged");
 		else
 		    ifo->options &= ~DHCPCD_STATIC;
 	}
diff --git a/src/dhcp6.c b/src/dhcp6.c
index c16eb542..59f1e8c6 100644
--- a/src/dhcp6.c
+++ b/src/dhcp6.c
@@ -44,6 +44,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <syslog.h>
 
 #define ELOOP_QUEUE	ELOOP_DHCP6
 #include "config.h"
@@ -1932,7 +1933,7 @@ dhcp6_checkstatusok(const struct interface *ifp,
 	void * (*f)(void *, size_t, uint16_t, uint16_t *), *farg;
 	char buf[32], *sbuf;
 	const char *status;
-	logfunc_t *logfunc;
+	int loglevel;
 
 	state = D6_STATE(ifp);
 	f = p ? dhcp6_findoption : dhcp6_findmoption;
@@ -1981,10 +1982,10 @@ dhcp6_checkstatusok(const struct interface *ifp,
 	}
 
 	if (state->lerror == code || state->state == DH6S_INIT)
-		logfunc = logdebugx;
+		loglevel = LOG_DEBUG;
 	else
-		logfunc = logerrx;
-	logfunc("%s: DHCPv6 REPLY: %s", ifp->name, status);
+		loglevel = LOG_ERR;
+	logmessage(loglevel, "%s: DHCPv6 REPLY: %s", ifp->name, status);
 	free(sbuf);
 	state->lerror = code;
 	errno = 0;
@@ -2824,16 +2825,16 @@ dhcp6_delegate_prefix(struct interface *ifp)
 			if (!(ap->flags & IPV6_AF_DELEGATEDPFX))
 				continue;
 			if (!(ap->flags & IPV6_AF_DELEGATEDLOG)) {
-				logfunc_t *logfunc;
+				int loglevel;
 
 				if (ap->flags & IPV6_AF_NEW)
-					logfunc = loginfox;
+					loglevel = LOG_INFO;
 				else
-					logfunc = logdebugx;
+					loglevel = LOG_DEBUG;
 				/* We only want to log this the once as we loop
 				 * through many interfaces first. */
 				ap->flags |= IPV6_AF_DELEGATEDLOG;
-				logfunc("%s: delegated prefix %s",
+				logmessage(loglevel, "%s: delegated prefix %s",
 				    ifp->name, ap->saddr);
 				ap->flags &= ~IPV6_AF_NEW;
 			}
@@ -2967,7 +2968,7 @@ dhcp6_bind(struct interface *ifp, const char *op, const char *sfrom)
 	struct dhcp6_state *state = D6_STATE(ifp);
 	bool timedout = (op == NULL), has_new = false, confirmed;
 	struct ipv6_addr *ia;
-	logfunc_t *lognewinfo;
+	int loglevel;
 	struct timespec now;
 
 	TAILQ_FOREACH(ia, &state->addrs, next) {
@@ -2976,9 +2977,9 @@ dhcp6_bind(struct interface *ifp, const char *op, const char *sfrom)
 			break;
 		}
 	}
-	lognewinfo = has_new ? loginfox : logdebugx;
+	loglevel = has_new ? LOG_INFO : LOG_DEBUG;
 	if (!timedout) {
-		lognewinfo("%s: %s received from %s", ifp->name, op, sfrom);
+		logmessage(loglevel, "%s: %s received from %s", ifp->name, op, sfrom);
 		/* If we delegated from an unconfirmed lease we MUST drop
 		 * them now. Hopefully we have new delegations. */
 		if (state->reason != NULL &&
@@ -3137,20 +3138,20 @@ dhcp6_bind(struct interface *ifp, const char *op, const char *sfrom)
 			dhcp6_deprecateaddrs(&state->addrs);
 
 		if (state->state == DH6S_INFORMED)
-			lognewinfo("%s: refresh in %"PRIu32" seconds",
+			logmessage(loglevel, "%s: refresh in %"PRIu32" seconds",
 			    ifp->name, state->renew);
 		else if (state->renew == ND6_INFINITE_LIFETIME)
-			lognewinfo("%s: leased for infinity", ifp->name);
+			logmessage(loglevel, "%s: leased for infinity", ifp->name);
 		else if (state->renew || state->rebind)
-			lognewinfo("%s: renew in %"PRIu32", "
+			logmessage(loglevel, "%s: renew in %"PRIu32", "
 			    "rebind in %"PRIu32", "
 			    "expire in %"PRIu32" seconds",
 			    ifp->name,
 			    state->renew, state->rebind, state->expire);
 		else if (state->expire == 0)
-			lognewinfo("%s: will expire", ifp->name);
+			logmessage(loglevel, "%s: will expire", ifp->name);
 		else
-			lognewinfo("%s: expire in %"PRIu32" seconds",
+			logmessage(loglevel, "%s: expire in %"PRIu32" seconds",
 			    ifp->name, state->expire);
 		rt_build(ifp->ctx, AF_INET6);
 		if (!confirmed && !timedout)
diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index a306bd6f..9bfcdc14 100644
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -48,6 +48,7 @@ const char dhcpcd_copyright[] = "Copyright (c) 2006-2020 Roy Marples";
 #include <string.h>
 #include <unistd.h>
 #include <time.h>
+#include <syslog.h>
 
 #include "config.h"
 #include "arp.h"
@@ -2177,11 +2178,11 @@ printpidfile:
 	}
 	if (ifp == NULL) {
 		if (ctx.ifc == 0) {
-			logfunc_t *logfunc;
+			int loglevel;
 
-			logfunc = ctx.options & DHCPCD_INACTIVE ?
-			    logdebugx : logerrx;
-			logfunc("no valid interfaces found");
+			loglevel = ctx.options & DHCPCD_INACTIVE ?
+			    LOG_DEBUG : LOG_ERR;
+			logmessage(loglevel, "no valid interfaces found");
 		} else
 			goto exit_failure;
 		if (!(ctx.options & DHCPCD_LINK)) {
@@ -2225,11 +2226,11 @@ printpidfile:
 		    ctx.options & DHCPCD_LINK &&
 		    !(ctx.options & DHCPCD_WAITIP))
 		{
-			logfunc_t *logfunc;
+			int loglevel;
 
-			logfunc = ctx.options & DHCPCD_INACTIVE ?
-			    logdebugx : logwarnx;
-			logfunc("no interfaces have a carrier");
+			loglevel = ctx.options & DHCPCD_INACTIVE ?
+			    LOG_DEBUG : LOG_WARNING;
+			logmessage(loglevel, "no interfaces have a carrier");
 			dhcpcd_daemonise(&ctx);
 		} else if (t > 0 &&
 		    /* Test mode removes the daemonise bit, so check for both */
diff --git a/src/if-linux.c b/src/if-linux.c
index 8f939761..e277524f 100644
--- a/src/if-linux.c
+++ b/src/if-linux.c
@@ -69,6 +69,7 @@
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <syslog.h>
 
 #include "config.h"
 #include "bpf.h"
@@ -1964,11 +1965,11 @@ if_setup_inet6(const struct interface *ifp)
 	snprintf(path, sizeof(path), "%s/%s/accept_ra", p_conf, ifp->name);
 	ra = check_proc_int(path);
 	if (ra == -1) {
-		logfunc_t *logfunc = errno == ENOENT? logdebug : logwarn;
+		int loglevel = errno == ENOENT? LOG_DEBUG : LOG_WARNING;
 
 		/* The sysctl probably doesn't exist, but this isn't an
 		 * error as such so just log it and continue */
-		logfunc("%s", path);
+		logerrmessage(loglevel, "%s", path);
 	} else if (ra != 0) {
 		if (if_writepathuint(ifp->ctx, path, 0) == -1)
 			logerr("%s: %s", __func__, path);
diff --git a/src/if.c b/src/if.c
index a4bf4d61..901a66f7 100644
--- a/src/if.c
+++ b/src/if.c
@@ -30,6 +30,7 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
+#include <syslog.h>
 
 #include "config.h"
 
@@ -462,8 +463,8 @@ if_discover(struct dhcpcd_ctx *ctx, struct ifaddrs **ifaddrs,
 #endif
 
 		if (if_vimaster(ctx, spec.devname) == 1) {
-			logfunc_t *logfunc = argc != 0 ? logerrx : logdebugx;
-			logfunc("%s: is a Virtual Interface Master, skipping",
+			int loglevel = argc != 0 ? LOG_ERR : LOG_DEBUG;
+			logmessage(loglevel, "%s: is a Virtual Interface Master, skipping",
 			    spec.devname);
 			continue;
 		}
diff --git a/src/ipv6.c b/src/ipv6.c
index 0ac9f095..85049554 100644
--- a/src/ipv6.c
+++ b/src/ipv6.c
@@ -36,6 +36,7 @@
 #include <net/route.h>
 #include <netinet/in.h>
 #include <netinet/if_ether.h>
+#include <syslog.h>
 
 #include "config.h"
 
@@ -616,7 +617,7 @@ ipv6_addaddr1(struct ipv6_addr *ia, const struct timespec *now)
 {
 	struct interface *ifp;
 	uint32_t pltime, vltime;
-	__printflike(1, 2) void (*logfunc)(const char *, ...);
+	int loglevel;
 #ifdef ND6_ADVERTISE
 	bool vltime_was_zero = ia->prefix_vltime == 0;
 #endif
@@ -675,8 +676,8 @@ ipv6_addaddr1(struct ipv6_addr *ia, const struct timespec *now)
 		}
 	}
 
-	logfunc = ia->flags & IPV6_AF_NEW ? loginfox : logdebugx;
-	logfunc("%s: adding %saddress %s", ifp->name,
+	loglevel = ia->flags & IPV6_AF_NEW ? LOG_INFO : LOG_DEBUG;
+	logmessage(loglevel, "%s: adding %saddress %s", ifp->name,
 #ifdef IPV6_AF_TEMPORARY
 	    ia->flags & IPV6_AF_TEMPORARY ? "temporary " : "",
 #else
diff --git a/src/ipv6nd.c b/src/ipv6nd.c
index eeea69af..75e29983 100644
--- a/src/ipv6nd.c
+++ b/src/ipv6nd.c
@@ -42,6 +42,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <syslog.h>
 
 #define ELOOP_QUEUE	ELOOP_IPV6ND
 #include "common.h"
@@ -1016,7 +1017,7 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 	bool new_rap, new_data, has_address;
 	uint32_t old_lifetime;
 	int ifmtu;
-	__printflike(1, 2) void (*logfunc)(const char *, ...);
+	int loglevel;
 #ifdef IPV6_MANAGETEMPADDR
 	uint8_t new_ap;
 #endif
@@ -1117,8 +1118,8 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 	 * much needless log spam. */
 	if (rap->willexpire)
 		new_data = true;
-	logfunc = new_data || !rap->isreachable ? loginfox : logdebugx,
-	logfunc("%s: Router Advertisement from %s", ifp->name, rap->sfrom);
+	loglevel = new_data || !rap->isreachable ? LOG_INFO : LOG_DEBUG,
+	logmessage(loglevel, "%s: Router Advertisement from %s", ifp->name, rap->sfrom);
 
 	clock_gettime(CLOCK_MONOTONIC, &rap->acquired);
 	rap->flags = nd_ra->nd_ra_flags_reserved;
@@ -1201,15 +1202,15 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 
 		switch (ndo.nd_opt_type) {
 		case ND_OPT_PREFIX_INFORMATION:
-			logfunc = new_data ? logerrx : logdebugx;
+			loglevel = new_data ? LOG_ERR : LOG_DEBUG;
 			if (ndo.nd_opt_len != 4) {
-				logfunc("%s: invalid option len for prefix",
+				logmessage(loglevel, "%s: invalid option len for prefix",
 				    ifp->name);
 				continue;
 			}
 			memcpy(&pi, p, sizeof(pi));
 			if (pi.nd_opt_pi_prefix_len > 128) {
-				logfunc("%s: invalid prefix len", ifp->name);
+				logmessage(loglevel, "%s: invalid prefix len", ifp->name);
 				continue;
 			}
 			/* nd_opt_pi_prefix is not aligned. */
@@ -1218,13 +1219,13 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 			if (IN6_IS_ADDR_MULTICAST(&pi_prefix) ||
 			    IN6_IS_ADDR_LINKLOCAL(&pi_prefix))
 			{
-				logfunc("%s: invalid prefix in RA", ifp->name);
+				logmessage(loglevel, "%s: invalid prefix in RA", ifp->name);
 				continue;
 			}
 			if (ntohl(pi.nd_opt_pi_preferred_time) >
 			    ntohl(pi.nd_opt_pi_valid_time))
 			{
-				logfunc("%s: pltime > vltime", ifp->name);
+				logmessage(loglevel, "%s: pltime > vltime", ifp->name);
 				continue;
 			}
 			TAILQ_FOREACH(ap, &rap->addrs, next)
@@ -1304,13 +1305,13 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 
 		case ND_OPT_MTU:
 			if (len < sizeof(mtu)) {
-				logfunc("%s: short MTU option", ifp->name);
+				logmessage(loglevel, "%s: short MTU option", ifp->name);
 				break;
 			}
 			memcpy(&mtu, p, sizeof(mtu));
 			mtu.nd_opt_mtu_mtu = ntohl(mtu.nd_opt_mtu_mtu);
 			if (mtu.nd_opt_mtu_mtu < IPV6_MMTU) {
-				logfunc("%s: invalid MTU %d",
+				logmessage(loglevel, "%s: invalid MTU %d",
 				    ifp->name, mtu.nd_opt_mtu_mtu);
 				break;
 			}
@@ -1318,7 +1319,7 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 			if (ifmtu == -1)
 				logerr("if_getmtu");
 			else if (mtu.nd_opt_mtu_mtu > (uint32_t)ifmtu) {
-				logfunc("%s: advertised MTU %d"
+				logmessage(loglevel, "%s: advertised MTU %d"
 				    " is greater than link MTU %d",
 				    ifp->name, mtu.nd_opt_mtu_mtu, ifmtu);
 				rap->mtu = (uint32_t)ifmtu;
@@ -1327,7 +1328,7 @@ ipv6nd_handlera(struct dhcpcd_ctx *ctx,
 			break;
 		case ND_OPT_RDNSS:
 			if (len < sizeof(rdnss)) {
-				logfunc("%s: short RDNSS option", ifp->name);
+				logmessage(loglevel, "%s: short RDNSS option", ifp->name);
 				break;
 			}
 			memcpy(&rdnss, p, sizeof(rdnss));
diff --git a/src/logerr.c b/src/logerr.c
index b33204d9..09ddceec 100644
--- a/src/logerr.c
+++ b/src/logerr.c
@@ -220,7 +220,7 @@ vlogmessage(int pri, const char *fmt, va_list args)
 #pragma GCC diagnostic pop
 #endif
 
-__printflike(2, 3) static void
+__printflike(2, 3) void
 logmessage(int pri, const char *fmt, ...)
 {
 	va_list args;
@@ -240,8 +240,18 @@ vlogerrmessage(int pri, const char *fmt, va_list args)
 	logmessage(pri, "%s: %s", buf, strerror(_errno));
 }
 
+__printflike(2, 3) void
+logerrmessage(int pri, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	vlogerrmessage(pri, fmt, args);
+	va_end(args);
+}
+
 void
-logdebug(const char *fmt, ...)
+log_debug(const char *fmt, ...)
 {
 	va_list args;
 
@@ -251,7 +261,7 @@ logdebug(const char *fmt, ...)
 }
 
 void
-logdebugx(const char *fmt, ...)
+log_debugx(const char *fmt, ...)
 {
 	va_list args;
 
@@ -261,7 +271,7 @@ logdebugx(const char *fmt, ...)
 }
 
 void
-loginfo(const char *fmt, ...)
+log_info(const char *fmt, ...)
 {
 	va_list args;
 
@@ -271,7 +281,7 @@ loginfo(const char *fmt, ...)
 }
 
 void
-loginfox(const char *fmt, ...)
+log_infox(const char *fmt, ...)
 {
 	va_list args;
 
@@ -281,7 +291,7 @@ loginfox(const char *fmt, ...)
 }
 
 void
-logwarn(const char *fmt, ...)
+log_warn(const char *fmt, ...)
 {
 	va_list args;
 
@@ -291,7 +301,7 @@ logwarn(const char *fmt, ...)
 }
 
 void
-logwarnx(const char *fmt, ...)
+log_warnx(const char *fmt, ...)
 {
 	va_list args;
 
@@ -301,7 +311,7 @@ logwarnx(const char *fmt, ...)
 }
 
 void
-logerr(const char *fmt, ...)
+log_err(const char *fmt, ...)
 {
 	va_list args;
 
@@ -311,7 +321,7 @@ logerr(const char *fmt, ...)
 }
 
 void
-logerrx(const char *fmt, ...)
+log_errx(const char *fmt, ...)
 {
 	va_list args;
 
diff --git a/src/logerr.h b/src/logerr.h
index 1923ab73..a7f034a7 100644
--- a/src/logerr.h
+++ b/src/logerr.h
@@ -39,17 +39,31 @@
 #endif
 #endif /* !__printflike */
 
-__printflike(1, 2) typedef void logfunc_t(const char *, ...);
-
-__printflike(1, 2) void logdebug(const char *, ...);
-__printflike(1, 2) void logdebugx(const char *, ...);
-__printflike(1, 2) void loginfo(const char *, ...);
-__printflike(1, 2) void loginfox(const char *, ...);
-__printflike(1, 2) void logwarn(const char *, ...);
-__printflike(1, 2) void logwarnx(const char *, ...);
-__printflike(1, 2) void logerr(const char *, ...);
+/* Please do not call log_* functions directly, use macros below */
+__printflike(1, 2) void log_debug(const char *, ...);
+__printflike(1, 2) void log_debugx(const char *, ...);
+__printflike(1, 2) void log_info(const char *, ...);
+__printflike(1, 2) void log_infox(const char *, ...);
+__printflike(1, 2) void log_warn(const char *, ...);
+__printflike(1, 2) void log_warnx(const char *, ...);
+__printflike(1, 2) void log_err(const char *, ...);
+__printflike(1, 2) void log_errx(const char *, ...);
 #define	LOGERROR	logerr("%s: %d", __FILE__, __LINE__)
-__printflike(1, 2) void logerrx(const char *, ...);
+
+__printflike(2, 3) void logmessage(int pri, const char *fmt, ...);
+__printflike(2, 3) void logerrmessage(int pri, const char *fmt, ...);
+
+/* These are macros to prevent taking address of them so
+ * __FILE__ / __LINE__ etc can easily be added.
+ * Also can't use __VA_OPT__ or ##__VA_ARGS for compatibility reasons */
+#define logdebug(...) log_debug(__VA_ARGS__)
+#define logdebugx(...) log_debugx(__VA_ARGS__)
+#define loginfo(...) log_info(__VA_ARGS__)
+#define loginfox(...) log_infox(__VA_ARGS__)
+#define logwarn(...) log_warn(__VA_ARGS__)
+#define logwarnx(...) log_warnx(__VA_ARGS__)
+#define logerr(...) log_err(__VA_ARGS__)
+#define logerrx(...) log_errx(__VA_ARGS__)
 
 void logsetopts(unsigned int);
 #define	LOGERR_DEBUG	(1U << 6)

Follow-Ups:
Re: dhcpcd logging subsystem modificationRoy Marples
References:
dhcpcd logging subsystem modificationSergey Nikiforov
Re: dhcpcd logging subsystem modificationRoy Marples
Re: dhcpcd logging subsystem modificationSergey Nikiforov
Re: dhcpcd logging subsystem modificationRoy Marples
Archive administrator: postmaster@marples.name