Index | Thread | Search

From:
Jan Klemkow <jan@openbsd.org>
Subject:
Re: newsyslog: support glob(3)
To:
Alvar Penning <post@0x21.biz>
Cc:
tech@openbsd.org
Date:
Wed, 11 Mar 2026 00:04:33 +0100

Download raw body.

Thread
On Tue, Jan 27, 2026 at 10:14:14PM +0100, Alvar Penning wrote:
> After using this patch for almost two years now, I wanted to bring it to
> your attention again. It introduces glob(3) support for logfile names in
> newsyslog(8), which is useful since some software likes to produce a
> multitude of logs, named after different channels.
> 
> While the diff might looks large on first glance, lots of it is just
> moving code into its own function, being called from two places now.

Hi Alvar,

I tested and reviewed your diff.  It works fine for me and I really
appreciate the globing of filenames here.

The general manpage improvements are also good.  But, should be part of
a separate diff.  Please, send a new diff, with just the globing part.
And I'm happy to OK this.

Thanks,
Jan

> diff --git a/newsyslog.8 b/newsyslog.8
> index 391c7c3d99d..7bf03027ad4 100644
> --- a/newsyslog.8
> +++ b/newsyslog.8
> @@ -170,7 +170,7 @@ By default, this configuration file is
>  Each line of the file contains information about a particular log file
>  that should be handled by
>  .Nm newsyslog .
> -Each line has five mandatory fields and up to three optional fields, with
> +Each line has five mandatory fields and up to four optional fields, with

I count five optional fields:

	owner:group       This optional field...
	flags             The optional flags field...
	pid_file          This optional field...
	signal            This optional field...
	command           This optional field...

and the "monitor" field also looks like kind of optional to me.

I would rephrase it as "... and several optional fields, ...".
So, we don't have to count it :-)

>  whitespace separating each field.
>  Blank lines or lines beginning with a hash mark
>  .Pq Ql #
> @@ -180,6 +180,9 @@ follows:
>  .Bl -tag -width XXXXXXXXXXXXXXXX
>  .It Ar logfile_name
>  The full pathname of the system log file to be archived.
> +This path might be a
> +.Xr glob 7
> +pattern, which will be expanded accordingly.
>  .It Ar owner:group
>  This optional field specifies the owner and group for the archive file.
>  The
> @@ -367,8 +370,13 @@ rotate on every 5th day of the month at 6:00 hr
>  .It Ar flags
>  The optional
>  .Ar flags
> -field specifies if the archives should have any special processing
> -done to the archived log files.
> +field specifies any special processing for each entry.
> +Multiple
> +.Ar flags
> +can be concatenated together.
> +.Pp
> +.Bl -tag -width Ds -compact -offset indent
> +.It Li Z
>  The
>  .Sq Z
>  flag will make the archive
> @@ -376,7 +384,9 @@ files compressed to save space using
>  .Xr gzip 1
>  or
>  .Xr compress 1 ,
> -depending on compilation options.
> +depending on compilation options, defaulting to
> +.Xr gzip 1 .
> +.It Li B
>  The
>  .Sq B
>  flag means that the file is a
> @@ -384,13 +394,16 @@ binary file, and so the ASCII message which
>  .Nm
>  inserts to indicate the fact that the logs have been turned over
>  should not be included.
> +.It Li M
>  The
>  .Sq M
>  flag marks this entry as a monitored
>  log file.
> +.It Li F
>  The
>  .Sq F
>  flag specifies that symbolic links should be followed.
> +.El
>  .It Ar monitor
>  Specify the username (or email address) that should receive notification
>  messages if this is a monitored log file.
> @@ -442,6 +455,7 @@ default configuration file
>  .Sh SEE ALSO
>  .Xr compress 1 ,
>  .Xr gzip 1 ,
> +.Xr glob 7 ,
>  .Xr syslog 3 ,
>  .Xr syslogd 8
>  .Sh AUTHORS
> diff --git a/newsyslog.c b/newsyslog.c
> index 0be4ed259b9..6264fdc4259 100644
> --- a/newsyslog.c
> +++ b/newsyslog.c
> @@ -88,6 +88,7 @@
>  #include <err.h>
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <glob.h>
>  #include <grp.h>
>  #include <limits.h>
>  #include <pwd.h>
> @@ -161,6 +162,7 @@ int	stat_suffix(char *, size_t, char *, struct stat *,
>  	    int (*)(const char *, struct stat *));
>  off_t	sizefile(struct stat *);
>  int	parse_file(struct entrylist *, int *);
> +int parse_entry(struct entrylist *, int *, int, struct conf_entry *);
>  time_t	parse8601(char *);
>  time_t	parseDWM(char *);
>  void	child_killer(int);
> @@ -470,13 +472,14 @@ int
>  parse_file(struct entrylist *list, int *nentries)
>  {
>  	char line[BUFSIZ], *parse, *q, *errline, *group, *tmp, *ep;
> -	struct conf_entry *working;
> -	struct stat sb;
> +	struct conf_entry *working, *working_glob;
> +	size_t i;
>  	int lineno = 0;
>  	int ret = 0;
>  	FILE *f;
>  	const char *errstr;
>  	long l;
> +	glob_t g;
>  
>  	if (strcmp(conf, "-") == 0)
>  		f = stdin;
> @@ -504,9 +507,6 @@ nextline:
>  		if (working->log == NULL)
>  			err(1, NULL);
>  
> -		if ((working->logbase = strrchr(working->log, '/')) != NULL)
> -			working->logbase++;
> -
>  		q = parse = missing_field(sob(++parse), errline, lineno);
>  		*(parse = son(parse)) = '\0';
>  		if ((group = strchr(q, ':')) != NULL ||
> @@ -739,57 +739,111 @@ nextline:
>  			goto nextline;
>  		}
>  
> -		/* If there is an arcdir, set working->backdir. */
> -		if (arcdir != NULL && working->logbase != NULL) {
> -			if (*arcdir == '/') {
> -				/* Fully qualified arcdir */
> -				working->backdir = arcdir;
> -			} else {
> -				/* arcdir is relative to log's parent dir */
> -				*(working->logbase - 1) = '\0';
> -				if ((asprintf(&working->backdir, "%s/%s",
> -				    working->log, arcdir)) == -1)
> -					err(1, NULL);
> -				*(working->logbase - 1) = '/';
> -			}
> -			/* Ignore arcdir if it doesn't exist. */
> -			if (stat(working->backdir, &sb) != 0 ||
> -			    !S_ISDIR(sb.st_mode)) {
> -				if (working->backdir != arcdir)
> -					free(working->backdir);
> -				working->backdir = NULL;
> -			}
> -		} else
> -			working->backdir = NULL;
> -
> -		/* Make sure we can't oflow PATH_MAX */
> -		if (working->backdir != NULL) {
> -			if (snprintf(line, sizeof(line), "%s/%s.%d%s",
> -			    working->backdir, working->logbase,
> -			    working->numlogs, COMPRESS_POSTFIX) >= PATH_MAX) {
> -				warnx("%s:%d: pathname too long: %s"
> -				    " --> skipping", conf, lineno, q);
> +		if (strpbrk(working->log, "*?[")) {
> +			/*
> +			 * This case also matches for escaped glob characters. However, the
> +			 * glob(3) function itself honors escape characters, thus resulting
> +			 * to a globed file list with only the desired logfile.
> +			 */
> +			if (glob(working->log, GLOB_NOSORT, NULL, &g)) {
> +				warnx("%s:%d: cannot glob(3), %s --> skipping",
> +					conf, lineno, strerror(errno));
>  				ret = 1;
>  				goto nextline;
>  			}
> -		} else {
> -			if (snprintf(line, sizeof(line), "%s.%d%s",
> -			    working->log, working->numlogs, COMPRESS_POSTFIX)
> -			    >= PATH_MAX) {
> -				warnx("%s:%d: pathname too long: %s"
> -				    " --> skipping", conf, lineno,
> -				    working->log);
> +			DPRINTF(("%s:%d: expanded glob %s to %zu files\n",
> +			    conf, lineno, working->log, g.gl_pathc));
> +
> +			for (i = 0; i < g.gl_pathc; i++) {
> +				working_glob = malloc(sizeof(*working));
> +				if (working_glob == NULL)
> +					err(1, NULL);
> +
> +				memcpy(working_glob, working, sizeof(*working));
> +
> +				if ((working_glob->log = strdup(g.gl_pathv[i])) == NULL)
> +					err(1, NULL);
> +
> +				if (parse_entry(list, nentries, lineno, working_glob)) {
> +					ret = 1;
> +					goto nextline;
> +				}
> +			}
> +
> +			/*
> +			 * It is not necessary to duplicate and free the other three char
> +			 * pointers in conf_entry as those values are identical for each
> +			 * globed file. Even more important, later on they will only be
> +			 * referenced in a reading fashion.
> +			 */
> +			free(working->log);
> +			free(working);
> +
> +			globfree(&g);
> +		} else
> +			if (parse_entry(list, nentries, lineno, working)) {
>  				ret = 1;
>  				goto nextline;
>  			}
> -		}
> -		TAILQ_INSERT_TAIL(list, working, next);
> -		(*nentries)++;
>  	}
>  	(void)fclose(f);
>  	return (ret);
>  }
>  
> +int
> +parse_entry(
> +	struct entrylist *list, int *nentries,
> +	int lineno, struct conf_entry *working
> +)
> +{
> +	char line[BUFSIZ];
> +	int linelen;
> +	struct stat sb;
> +
> +	if ((working->logbase = strrchr(working->log, '/')) != NULL)
> +		working->logbase++;
> +
> +	/* If there is an arcdir, set working->backdir. */
> +	if (arcdir != NULL && working->logbase != NULL) {
> +		if (*arcdir == '/') {
> +			/* Fully qualified arcdir */
> +			working->backdir = arcdir;
> +		} else {
> +			/* arcdir is relative to log's parent dir */
> +			*(working->logbase - 1) = '\0';
> +			if ((asprintf(&working->backdir, "%s/%s",
> +			    working->log, arcdir)) == -1)
> +				err(1, NULL);
> +			*(working->logbase - 1) = '/';
> +		}
> +		/* Ignore arcdir if it doesn't exist. */
> +		if (stat(working->backdir, &sb) != 0 ||
> +		    !S_ISDIR(sb.st_mode)) {
> +			if (working->backdir != arcdir)
> +				free(working->backdir);
> +			working->backdir = NULL;
> +		}
> +	} else
> +		working->backdir = NULL;
> +
> +	/* Make sure we can't oflow PATH_MAX */
> +	if (working->backdir != NULL)
> +		linelen = snprintf(line, sizeof(line), "%s/%s.%d%s",
> +		    working->backdir, working->logbase,
> +		    working->numlogs, COMPRESS_POSTFIX);
> +	else
> +		linelen = snprintf(line, sizeof(line), "%s.%d%s",
> +		    working->log, working->numlogs, COMPRESS_POSTFIX);
> +	if (linelen < 0 || linelen >= sizeof(line) || linelen >= PATH_MAX) {
> +		warnx("%s:%d: pathname too long: %s --> skipping", conf, lineno, line);
> +		return 1;
> +	}
> +
> +	TAILQ_INSERT_TAIL(list, working, next);
> +	(*nentries)++;
> +	return 0;
> +}
> +
>  char *
>  missing_field(char *p, char *errline, int lineno)
>  {
>