Index | Thread | Search

From:
"Alvar Penning" <post@0x21.biz>
Subject:
Re: newsyslog: support glob(3)
To:
"Stuart Henderson" <stu@spacehopper.org>
Cc:
"Jan Klemkow" <jan@openbsd.org>, <tech@openbsd.org>
Date:
Mon, 16 Mar 2026 21:22:02 +0100

Download raw body.

Thread
Hi Stuart,

On Sat Mar 14, 2026 at 1:39 AM CET, Stuart Henderson wrote:
> I would quite like to have this feature and it seems a pretty clean
> implementation. I have some small nits inline n the diff, but there's
> one slight issue that is a bit trickier.

Thanks for your reply and your input.

In the following new diff, I have addressed these comments.

> A convenient way to use this would be to archive all files in a dir,
> e.g. /var/log/consoles/*. However, previously archived files are
> candidates for rotation too. Say you have rotation at 250K and an
> existing file that's much larger than this, every run of newsyslog
> it will now get rotated again,
>
> somelog.1.gz
> somelog.1.gz.0.gz
> somelog.1.gz.0.gz.0.gz
> somelog.1.gz.0.gz.0.gz.0.gz
> somelog.1.gz.0.gz.0.gz.0.gz.0.gz
> somelog.1.gz.0.gz.0.gz.0.gz.0.gz.0.gz
> somelog.1.gz.0.gz.0.gz.0.gz.0.gz.0.gz.0.gz
>
> and on until you hit PATH_MAX.
>
> On the one hand, the user gets exactly what they ask for, but on the
> other, avoiding this is difficult with a simple glob. (I don't really
> want regexps in config either though). Any idea how to avoid this?
> Would it make sense to just skip files with the typical archival
> suffixes automatically?

I remember thinking about this when I first wrote these changes. But
then went on using a pattern like "/somelog*.log" and putting these
concerns under "user configuration issues".

However, this is definitely a footgun. You are totally right.

In the new diff, newsyslog checks if a globbed path has a suffix
indicating this file is a rotation, also taking the 'Z' compression flag
into account.

Please feel free to take another look and thanks for the feedback.

Best,
Alvar


diff --git a/newsyslog.8 b/newsyslog.8
index 391c7c3d99d..e57552b304c 100644
--- a/newsyslog.8
+++ b/newsyslog.8
@@ -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
@@ -442,6 +445,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..e5ecc3a5316 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,8 @@ 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	is_glob_rotation(struct conf_entry *, const char *);
+int	parse_entry(struct entrylist *, int *, int, struct conf_entry *);
 time_t	parse8601(char *);
 time_t	parseDWM(char *);
 void	child_killer(int);
@@ -470,13 +473,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 +508,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 +740,144 @@ 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;
+		if (glob(working->log, GLOB_NOCHECK | GLOB_NOSORT, NULL, &g)) {
+			warnx("%s:%d: cannot glob(3), %s"
+			    " --> skipping",
+			    conf, lineno, strerror(errno));
+			ret = 1;
+			goto nextline;
+		}
 
-		/* 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);
-				ret = 1;
-				goto nextline;
+		for (i = 0; i < g.gl_pathc; i++) {
+			DPRINTF(("%s:%d: expanded logfile name \"%s\" to \"%s\""
+			    " (%zu/%zu)\n",
+			    conf, lineno, working->log, g.gl_pathv[i],
+			    i+1, g.gl_pathc));
+
+			if (is_glob_rotation(working, g.gl_pathv[i])) {
+				DPRINTF(("%s:%d: \"%s\" looks like a rotation"
+				    " --> skipping\n",
+				    conf, lineno, g.gl_pathv[i]));
+				continue;
 			}
-		} 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);
+
+			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;
+				/* break to free resources below */
+				break;
 			}
 		}
-		TAILQ_INSERT_TAIL(list, working, next);
-		(*nentries)++;
+
+		/*
+		 * Original working is duplicated into working_glob in the for
+		 * loop above and can be freed now. Every struct char pointer
+		 * next to log is identical in each duplicate and used later.
+		 */
+		free(working->log);
+		free(working);
+
+		globfree(&g);
 	}
 	(void)fclose(f);
 	return (ret);
 }
 
+int
+is_glob_rotation(struct conf_entry *working, const char *glob_line)
+{
+	size_t endpos = strlen(glob_line) - 1;
+
+	/* Ignore empty inputs or identical to the logfile name. */
+	if (*glob_line == '\0' || strcmp(working->log, glob_line) == 0)
+		return 0;
+
+	/* Suffix check for compressed files. */
+	if (working->flags & CE_COMPACT && endpos >= strlen(COMPRESS_POSTFIX)) {
+		if (strcmp(
+		           glob_line + endpos - (strlen(COMPRESS_POSTFIX) - 1),
+		           COMPRESS_POSTFIX
+		   ) != 0)
+			return 0;
+
+		endpos -= strlen(COMPRESS_POSTFIX);
+	}
+
+	/* Expect at least one digit. */
+	if (endpos == 0 || !isdigit(glob_line[endpos--]))
+		return 0;
+
+	for (; endpos > 0 && isdigit(glob_line[endpos]); endpos--);
+
+	/* Expect dot before digits. */
+	if (endpos == 0 || glob_line[endpos] != '.')
+		return 0;
+
+	return 1;
+}
+
+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)
 {