From: Nicolas Samberger Subject: Re: newsyslog: support glob(3) To: Alvar Penning Cc: Stuart Henderson , Jan Klemkow , Date: Wed, 18 Mar 2026 14:00:59 +0100 Am Mon, Mar 16, 2026 at 09:22:02PM +0100 schrieb Alvar Penning: > 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. Hi Alvar, Thanks for the diff. I tested the globbing with recursive filepathes and works for me. Some comments see below. Greeds Nico > diff --git a/newsyslog.c b/newsyslog.c > index 0be4ed259b9..e5ecc3a5316 100644 > --- a/newsyslog.c > +++ b/newsyslog.c > @@ -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) according to style(9) it looks a bit odd should be something like this: 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 > +) same here: 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) > { >