From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050720 Fedora/1.0.6-1.1.fc3 Firefox/1.0.6 Description of problem: Logrotate should continue rotating logs even if there are some non-fatal errors on the configuration files. The most common reason I have problems with is: "log file was listed more than once in the configuration". In this specific case, I guess it should apply the first rule it finds in the configuration, ignoring the other one. Frankly, it could implement whatever, the first, the last, the most restrictive, the less restrictive, apply all of them one after the other, or even not apply any rotation for *THIS* log only!!! (this last one I dislike a bit, but it's better than what logrotate does now). What I think it's inadmissable is that logrotate stops rotating *ALL* logs because of some error in configuration of one of them. You may say the problem is with me, and that I did the wrong configuration, but it happens a lot that I configure the rotating for some log in /etc/logrotate.conf, and then I ask the developpers to include logrotation in a file in /etc/logrotate.d/ in the RPM package. When I upgrade the RPM, if I forget changing /etc/logrotate.conf again (sometimes another guy did it, so I'm not always sure about that), and that's it, logrotate stops working and my hosts fill their disks with logs!!! It's not always obvious that I have to check logrotate, after all, all I did was install an RPM (it could even be automated install with yum). Version-Release number of selected component (if applicable): logrotate-3.7.1-2 How reproducible: Always Steps to Reproduce: 1. cp /etc/logrotate.d/syslog /etc/logrotate.d/stopworking 2. /usr/sbin/logrotate /etc/logrotate.conf Actual Results: # /usr/sbin/logrotate /etc/logrotate.conf error: syslog:1 duplicate log entry for /var/log/messages Expected Results: Logrotate should issue warnings about duplicate log entries, but should rotate the logs anyway. Additional info:
Created attachment 134926 [details] A patch fixing few issues with config file parsing The patch fixes this bug, #185538 and #182062. It's very simple - if logrotate finds a duplicate filename (or corrup config file), it skips it and continues with the next one.
Thnx. for patch, but it doesn't fix all cases. If there is a bug in local scope of configuration file, log file won't be skipped. example: $ cat rot/foo2 create missingok /tmp/foo2 { rotate -3 } $sudo logrotate-3.7.4/logrotate -vf rotFile ... reading config info for /tmp/foo2 error: foo2:4 bad rotation count '-3' error: found error in foo2, skipping reading config file foo3 .... rotating pattern: /tmp/foo2 forced from command line (-3 rotations) empty log files are rotated, old logs are removed considering log /tmp/foo2 log needs rotating rotating log /tmp/foo2, log->rotateCount is -3 log /tmp/foo2.-2 doesn't exist -- won't try to dispose of it renaming /tmp/foo2 to /tmp/foo2.1 creating new log mode = 0664 uid = 500 gid = 500 ...
Created attachment 135366 [details] A patch fixing few issues with config file parsing New version of the patch.
Comment on attachment 135366 [details] A patch fixing few issues with config file parsing >--- logrotate-3.7.4/config.c.old 2006-07-24 14:08:04.000000000 +0200 >+++ logrotate-3.7.4/config.c 2006-09-01 11:56:52.000000000 +0200 >@@ -267,10 +267,8 @@ > > if (readConfigFile(namelist[i], defConfig, logsPtr, > numLogsPtr)) { >- fchdir(here); >- close(here); >- free_2d_array(namelist, files_count); >- return 1; >+ message(MESS_ERROR, "found error in %s, skipping\n", namelist[i]); >+ continue; > } > } > >@@ -364,6 +362,7 @@ > glob_t globResult; > const char **argv; > int argc, argNum; >+ int logerror = 0; > > /* FIXME: createOwner and createGroup probably shouldn't be fixed > length arrays -- of course, if we aren't run setuid it doesn't >@@ -417,6 +416,34 @@ > > start = buf; > while (*start) { >+ if (logerror) { >+ while (*start != '}') { >+ if (*start == 0) { >+ message(MESS_ERROR, "%s:%d } expected \n", configFile, lineNum); >+ return 1; >+ } >+ else if (*start == '\n') { >+ lineNum++; >+ } >+ start++; >+ } >+ >+ if (newlog->files) >+ free(newlog->files); >+ if (newlog->pre && newlog->pre != defConfig->pre) >+ free(newlog->pre); >+ if (newlog->post && newlog->post != defConfig->post) >+ free(newlog->post); >+ if (newlog->first && newlog->first != defConfig->first) >+ free(newlog->first); >+ if (newlog->last && newlog->last != defConfig->last) >+ free(newlog->last); >+ (*numLogsPtr)--; >+ *logsPtr = realloc(*logsPtr, sizeof(**logsPtr) * *numLogsPtr); >+ newlog = defConfig; >+ >+ logerror = 0; >+ } > while (isblank(*start) && (*start)) > start++; > if (*start == '#') { >@@ -549,7 +576,12 @@ > if (rc == 4) { > message(MESS_ERROR, "%s:%d extra arguments for " > "create\n", configFile, lineNum); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > if (rc > 0) >@@ -560,7 +592,12 @@ > if (!pw) { > message(MESS_ERROR, "%s:%d unknown user '%s'\n", > configFile, lineNum, createOwner); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > newlog->createUid = pw->pw_uid; > endpwent(); >@@ -570,7 +607,12 @@ > if (!group) { > message(MESS_ERROR, "%s:%d unknown group '%s'\n", > configFile, lineNum, createGroup); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > newlog->createGid = group->gr_gid; > endgrent(); >@@ -605,7 +647,12 @@ > } else if (!isdigit(start[length])) { > message(MESS_ERROR, "%s:%d unknown unit '%c'\n", > configFile, lineNum, start[length]); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } else { > multiplier = 1; > } >@@ -614,7 +661,12 @@ > if (*chptr) { > message(MESS_ERROR, "%s:%d bad size '%s'\n", > configFile, lineNum, start); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > if (!strcmp(opt, "size")) { >@@ -676,7 +728,12 @@ > message(MESS_ERROR, > "%s:%d bad rotation count '%s'\n", > configFile, lineNum, start); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > *endtag = oldchar, start = endtag; > } >@@ -692,7 +749,12 @@ > if (*chptr || newlog->logStart < 0) { > message(MESS_ERROR, "%s:%d bad start count '%s'\n", > configFile, lineNum, start); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > *endtag = oldchar, start = endtag; > } >@@ -708,7 +770,12 @@ > if (*chptr || newlog->rotateAge < 0) { > message(MESS_ERROR, "%s:%d bad maximum age '%s'\n", > configFile, lineNum, start); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > *endtag = oldchar, start = endtag; > } >@@ -720,7 +787,12 @@ > *endtag = oldchar, start = endtag; > if (!(newlog->logAddress = readAddress(configFile, lineNum, > "mail", &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > } else if (!strcmp(start, "nomail")) { > /* hmmm, we could lose memory here, but not much */ >@@ -773,7 +845,12 @@ > "%s:%d tabooext may not appear inside " > "of log file definition\n", configFile, > lineNum); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > *endtag = oldchar, start = endtag; >@@ -819,7 +896,12 @@ > "%s:%d include may not appear inside " > "of log file definition\n", configFile, > lineNum); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > *endtag = oldchar, start = endtag; >@@ -831,7 +913,12 @@ > > if (readConfigPath(start, defConfig, logsPtr, > numLogsPtr)) >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > > *endtag = oldchar, start = endtag; > } >@@ -839,7 +926,12 @@ > *endtag = oldchar, start = endtag; > if (!(newlog->oldDir = readPath(configFile, lineNum, > "olddir", &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > #if 0 > if (stat(newlog->oldDir, &sb)) { >@@ -881,7 +973,12 @@ > if (! > (newlog->compress_prog = > readPath(configFile, lineNum, "compress", &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > if (access(newlog->compress_prog, X_OK)) { >@@ -889,7 +986,12 @@ > "%s:%d compression program %s is not an executable file\n", > configFile, lineNum, newlog->compress_prog); > free(newlog->compress_prog); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > message(MESS_DEBUG, "compress_prog is now %s\n", >@@ -901,7 +1003,12 @@ > (newlog->uncompress_prog = > readPath(configFile, lineNum, "uncompress", > &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > if (access(newlog->uncompress_prog, X_OK)) { >@@ -909,7 +1016,12 @@ > "%s:%d uncompression program %s is not an executable file\n", > configFile, lineNum, newlog->uncompress_prog); > free(newlog->uncompress_prog); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > message(MESS_DEBUG, "uncompress_prog is now %s\n", >@@ -923,7 +1035,12 @@ > (options = > readPath(configFile, lineNum, "compressoptions", > &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > if (poptParseArgvString(options, >@@ -933,7 +1050,12 @@ > "%s:%d invalid compression options\n", > configFile, lineNum); > free(options); >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > message(MESS_DEBUG, "compress_options is now %s\n", >@@ -945,7 +1067,12 @@ > (newlog->compress_ext = > readPath(configFile, lineNum, "compress-ext", > &start))) { >- return 1; >+ if (newlog != defConfig) { >+ logerror = 1; >+ continue; >+ } else { >+ return 1; >+ } > } > > message(MESS_DEBUG, "compress_ext is now %s\n", >@@ -973,7 +1100,8 @@ > if (newlog != defConfig) { > message(MESS_ERROR, "%s:%d unexpected log filename\n", > configFile, lineNum); >- return 1; >+ logerror = 1; >+ continue; > } > > /* If no compression options were found in config file, >@@ -1042,11 +1170,10 @@ > if (!strcmp((*logsPtr)[j].files[k], > globResult.gl_pathv[i])) { > message(MESS_ERROR, >- "%s:%d duplicate log entry for %s\n", >+ "%s:%d duplicate log entry for %s, skipping\n", > configFile, lineNum, > globResult.gl_pathv[i]); >- globfree(&globResult); >- return 1; >+ continue; > } > } > } >@@ -1067,7 +1194,7 @@ > start = endtag + 1; > } else if (*start == '}') { > if (newlog == defConfig) { >- message(MESS_ERROR, "%s:%d unxpected }\n", configFile, >+ message(MESS_ERROR, "%s:%d unexpected }\n", configFile, > lineNum); > return 1; > }
Created attachment 135370 [details] A patch fixing few issues with config file parsing Sorry about the above post (my bugzilla skill is not good enough).
Created attachment 135492 [details] A new version of the patch The patch fixes these issues: a) When a config file is corrupt, logrotate skips it and rolls back all changes. b) When a filename in log config is duplicate, logrotate skips it. c) When a log config is corrupt, logrotate skips it.
Created attachment 135558 [details] New version of the patch New fix - now when logrotate finds duplicate filename, it skips the whole log config.
Created attachment 135738 [details] another fix candidate This is adjusted grubber's patch, it was necessary to fix memory leak .
Created attachment 135775 [details] Another new version of the patch More fixes, including Peter's changes.
Created attachment 136992 [details] New version of the patch Another version of the patch, now without memory leaks.
thnx. to Jan Cholasta fixed in: FC6(testing) logrotate-3.7.4-10.fc6, rawhide logrotate-3.7.4-11.fc7.