Index | Thread | Search

From:
Kirill A. Korinsky <kirill@korins.ky>
Subject:
Re: smtpd: Give filters their own syslog tag
To:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Cc:
tech@openbsd.org
Date:
Mon, 09 Mar 2026 12:09:33 +0100

Download raw body.

Thread
On Fri, 20 Feb 2026 12:44:16 +0100,
Martijn van Duren <openbsd+tech@list.imperialat.at> wrote:
> 
> On 2/20/26 10:18, Martijn van Duren wrote:
> > EHLO,
> > 
> > A few months ago Leo Unglaub send me an e-mail asking about the position
> > of the mail ID inside the syslog line, more specifically that we
> > currently prepend the filter-name before it; Making it hard to find the
> > ID with automated tools.
> > 
> > While I'm not a fan of automated parsing of log-files, there is
> > something to say for having consistency in the placement, and not
> > forcing in the filtername.
> > 
> > I would like to propose the following diff, making use of syslog_r to
> > give every filter their own tag. Since tags can only have alpha-
> > and up to 32 characters (RFC3164 section 4.1.3) I made it the default
> > to use the first 32 alpha-characters from the filter-name, and if that
> > result is too ugly an admin can use the new tag keyword to create their
> > own one.
> > 
> > syslog_r isn't a portable function, but I've already discussed a couple
> > of potential solutions with op@, so there shouldn't be any problems in
> > that area.
> > 
> > OK?
> > 
> > martijn@
> As a followup I would like to add the option to let filters specify the
> loglevel per message. There's no value in specifying the global loglevel
> on a per-filter basis via cli flags, and to let smtpd log them all as
> LOG_WARNING.
> It might be worthwhile to also send a config update when smtpctl
> changes the loglevel to reduce the logmsg overhead for filters that
> would support this, but that wouldn't make a functional difference and
> outside the scope of this diff.
> 
> martijn@
>

It reads well, and, frankly, I had missed that feature when developed and
debuged filter a few times.

> diff d28c12b27e3c9e7a32cd929c1010839a90f63a6a 7e4994f30824f376c9581b937512172f7f3c1b9d
> commit - d28c12b27e3c9e7a32cd929c1010839a90f63a6a
> commit + 7e4994f30824f376c9581b937512172f7f3c1b9d
> blob - f10652c2a482e0f9fbbf3d842e7389939eb8471a
> blob + dae25dd933fae3a133a7efc9736907a5c296c446
> --- usr.sbin/smtpd/lka_filter.c
> +++ usr.sbin/smtpd/lka_filter.c
> @@ -25,7 +25,7 @@
>  #include "smtpd.h"
>  #include "log.h"
>  
> -#define	PROTOCOL_VERSION	"0.7"
> +#define	PROTOCOL_VERSION	"0.8"
>  
>  struct filter;
>  struct filter_session;
> @@ -323,7 +323,28 @@ processor_errfd(struct io *io, int evt, void *arg)
>  	switch (evt) {
>  	case IO_DATAIN:
>  		while ((line = io_getline(io, &len)) != NULL) {
> -			log_warnx_r(&processor->sd, "%s", line);
> +			if (!strncasecmp(
> +			    line, "DEBUG|", sizeof("DEBUG|") - 1))
> +				log_debug_r(&processor->sd, "%s",
> +				    line + sizeof("DEBUG|") - 1);
> +			else if (!strncasecmp(
> +			    line, "INFO|", sizeof("INFO|") - 1))
> +				log_info_r(&processor->sd, "%s",
> +				    line + sizeof("INFO|") - 1);
> +			else if (!strncasecmp(
> +			    line, "WARNING|", sizeof("WARNING|") - 1))
> +				log_warnx_r(&processor->sd, "%s",
> +				    line + sizeof("WARNING|") - 1);
> +			else if (!strncasecmp(
> +			    line, "WARN|", sizeof("WARN|") - 1))
> +				log_warnx_r(&processor->sd, "%s",
> +				    line + sizeof("WARN|") - 1);
> +			else if (!strncasecmp(
> +			    line, "FATAL|", sizeof("FATAL|") - 1))
> +				logit_r(&processor->sd, LOG_CRIT, "%s",
> +				    line + sizeof("FATAL|") - 1);
> +			else
> +				log_warnx_r(&processor->sd, "%s", line);
>  		}
>  	}
>  }
> blob - e9ca3ae6ff21edda39ae9dddb77d074f0b32b158
> blob + 65516d91b5cbf4b9202f89c2bcf4bac7d9277635
> --- usr.sbin/smtpd/log.c
> +++ usr.sbin/smtpd/log.c
> @@ -75,6 +75,16 @@ logit(int pri, const char *fmt, ...)
>  }
>  
>  void
> +logit_r(struct syslog_data *data, int pri, const char *fmt, ...)
> +{
> +	va_list	ap;
> +
> +	va_start(ap, fmt);
> +	vlog_r(data, pri, fmt, ap);
> +	va_end(ap);
> +}
> +
> +void
>  vlog(int pri, const char *fmt, va_list ap)
>  {
>  	vlog_r(NULL, pri, fmt, ap);
> @@ -160,6 +170,16 @@ log_info(const char *emsg, ...)
>  }
>  
>  void
> +log_info_r(struct syslog_data *data, const char *emsg, ...)
> +{
> +	va_list	 ap;
> +
> +	va_start(ap, emsg);
> +	vlog_r(data, LOG_INFO, emsg, ap);
> +	va_end(ap);
> +}
> +
> +void
>  log_debug(const char *emsg, ...)
>  {
>  	va_list	 ap;
> @@ -171,6 +191,18 @@ log_debug(const char *emsg, ...)
>  	}
>  }
>  
> +void
> +log_debug_r(struct syslog_data *data, const char *emsg, ...)
> +{
> +	va_list	 ap;
> +
> +	if (verbose > 1) {
> +		va_start(ap, emsg);
> +		vlog_r(data, LOG_DEBUG, emsg, ap);
> +		va_end(ap);
> +	}
> +}
> +
>  static void
>  vfatalc(int code, const char *emsg, va_list ap)
>  {
> blob - c8da7731c0b6f4d510f3976a97291cecf8d39560
> blob + da07847993c675bdbdae2d198412264df0ef05d8
> --- usr.sbin/smtpd/log.h
> +++ usr.sbin/smtpd/log.h
> @@ -35,10 +35,16 @@ void	log_warnx_r(struct syslog_data *, const char *, .
>  	    __attribute__((__format__ (printf, 2, 3)));
>  void	log_info(const char *, ...)
>  	    __attribute__((__format__ (printf, 1, 2)));
> +void	log_info_r(struct syslog_data *, const char *, ...)
> +	    __attribute__((__format__ (printf, 2, 3)));
>  void	log_debug(const char *, ...)
>  	    __attribute__((__format__ (printf, 1, 2)));
> +void	log_debug_r(struct syslog_data *, const char *, ...)
> +	    __attribute__((__format__ (printf, 2, 3)));
>  void	logit(int, const char *, ...)
>  	    __attribute__((__format__ (printf, 2, 3)));
> +void	logit_r(struct syslog_data *, int, const char *, ...)
> +	    __attribute__((__format__ (printf, 3, 4)));
>  void	vlog(int, const char *, va_list)
>  	    __attribute__((__format__ (printf, 2, 0)));
>  void	vlog_r(struct syslog_data *, int, const char *, va_list)
> 

-- 
wbr, Kirill