Ross Boylan
2004-Sep-17 05:58 UTC
[Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning
Package: logcheck Version: 1.2.25 Severity: normal Unless my understanding of shell scripts is off, there seem to be quite a few problems with the greplogoutput routine. First, individual sections of the code contain comments that clearly do not match what the code is doing. Second, the overall sequencing and flow of the tests (as written--perhaps less so as commented) doesn't make sense. Consider # Now ignore all entries from the logcheck-<package> files if [ -n "$ignore" ]; then debug "Applying Logcheck override files" for file in $(ls -1 $ignore/) ; do debug "clean logcheck-<package>: $file" cleanchecked "$ignore/$file" done else The code is actually using every single ignore file, though the comments say it should be using only the logcheck-* files. Given that, the subsequent efforts to pick out various subsets of the ignore files are completely redundant. Now even if only logcheck-* were run, the subsequent section debug "Cleaning logcheck" # Remove any entries already reported for file in $(ls $raise/ | grep -v '^logcheck') ; do debug "Cleaning logcheck: $file" cleanchecked "$raise/$file" done doesn't make much sense. The very first action of greplogoutput is to look for an ignore file of the same name, which in this case must be logcheck. The previous code, if it followed the comments, would have got all the logcheck-* files. So this section redoes all those things that have already been done. It's only "contribution" is to pick up logcheck* files that aren't logcheck-*, and it's not clear to me that's desirable (or that there are likely to be any such files). Finally, the overall logic according to the comments seems unnecessarily complex. For every package foo, the ignore files named foo logcheck-foo local local-* are ignored. Why not local-foo instead of local-*? Then there is the added wrinkle of the separate handling when the "raise" file name is logcheck. I encourage you to decide what you want this code to do and then make the code and the comments match that behavior. Finally, *please* expand the manual page to document what the behavior is. My patch in bug #215640 provides such a revised man page. I believe it reflected the actual behavior of the code at one point; there may have been changes since, and it will need to reflect whatever decisions are made dealing with the issues raised above. I really would like to avoid the necessity of rereading the code every time I want to figure out how to fill in the configuration files :) -- System Information: Debian Release: 3.1 APT prefers testing APT policy: (990, 'testing'), (50, 'unstable') Architecture: i386 (i686) Kernel: Linux 2.4.26advncdfs Locale: LANG=en_US, LC_CTYPE=en_US Versions of packages logcheck depends on: ii adduser 3.59 Add and remove users and groups ii cron 3.0pl1-86 management of regular background p ii debconf [debconf 1.4.30.3 Debian configuration management sy ii debianutils 2.8.4 Miscellaneous utilities specific t ii exim [mail-trans 3.36-11 An MTA (Mail Transport Agent) ii lockfile-progs 0.1.10 Programs for locking and unlocking ii logcheck-databas 1.2.25 A database of system log rules for ii logtail 1.2.25 Print log file lines that have not ii mailx 1:8.1.2-0.20040524cvs-1 A simple mail user agent ii perl 5.8.4-2 Larry Wall's Practical Extraction ii sysklogd [system 1.4.1-15 System Logging Daemon -- debconf information: * logcheck/security_level: workstation * logcheck/manage_conffiles: true * logcheck/auto_create_logfiles: true logcheck/upgrade-note: * logcheck/changes: * logcheck/install-note: * logcheck/rules-directories-note: * logcheck/email_address: root * logcheck/rewrite-note:
maks attems
2004-Sep-18 21:17 UTC
Bug#272047: [Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning
severity 272047 wishlist thanks On Thu, 16 Sep 2004, Ross Boylan wrote:> Package: logcheck > Version: 1.2.25 > Severity: normal > > Unless my understanding of shell scripts is off, there seem to be > quite a few problems with the greplogoutput routine. First, > individual sections of the code contain comments that clearly do not > match what the code is doing. Second, the overall sequencing and flow > of the tests (as written--perhaps less so as commented) doesn't make > sense.greplogoutput is completly _b0rked_, that's the only point i agree with you. i tried to rewrite it at some point in june, but failed somehow. with sarge approaching we opted for a surprisingly working redundancy. (we is meant for the debian logcheck team).> Consider > # Now ignore all entries from the logcheck-<package> files > if [ -n "$ignore" ]; then > debug "Applying Logcheck override files" > for file in $(ls -1 $ignore/) ; do > debug "clean logcheck-<package>: $file" > cleanchecked "$ignore/$file" > done > else > The code is actually using every single ignore file, though the > comments say it should be using only the logcheck-* files. > > Given that, the subsequent efforts to pick out various subsets of the > ignore files are completely redundant. > > Now even if only logcheck-* were run, the subsequent section > > debug "Cleaning logcheck" > # Remove any entries already reported > for file in $(ls $raise/ | grep -v '^logcheck') ; do > debug "Cleaning logcheck: $file" > cleanchecked "$raise/$file" > done > doesn't make much sense. The very first action of greplogoutput is to > look for an ignore file of the same name, which in this case must be > logcheck. The previous code, if it followed the comments, would have > got all the logcheck-* files. So this section redoes all those things > that have already been done. It's only "contribution" is to pick up > logcheck* files that aren't logcheck-*, and it's not clear to me > that's desirable (or that there are likely to be any such files).hmm i've longer no more looked at this function, but your analysis is wrong! the first for loop picks file out of $ignore and the second out of $raise that's a big difference!> Finally, the overall logic according to the comments seems > unnecessarily complex. For every package foo, the ignore files named > foo > logcheck-foo > local > local-* > are ignored. Why not local-foo instead of local-*?there could be strings raised by logcheck-postfix that are ignored by local-postdrop or whatever.> Then there is the added wrinkle of the separate handling when the > "raise" file name is logcheck. > > I encourage you to decide what you want this code to do and then make > the code and the comments match that behavior. Finally, *please* > expand the manual page to document what the behavior is. My patch in > bug #215640 provides such a revised man page. I believe it reflected > the actual behavior of the code at one point; there may have been > changes since, and it will need to reflect whatever decisions are made > dealing with the issues raised above.you are welcome to send patches to clean off greplogoutput, seperated patches for comment fixes and code cleanup are welcome. needless to say that they should be well tested and work on different setups! i already closed your above named bug, because of the strange confusion in your proposed manpage. good bits like the FILES section and discussion of *all* logchek options is already done! after reading this bug report i'm even less inclined to look at your misinterpretations.> I really would like to avoid the necessity of rereading the code every > time I want to figure out how to fill in the configuration files :)/usr/share/doc/logcheck-database/README.logcheck-database.gz should be enough. -- maks
Debian Bug Tracking System
2004-Sep-18 21:33 UTC
Processed: Re: [Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning
Processing commands for control at bugs.debian.org:> severity 272047 wishlistBug#272047: logcheck's greplogoutput code needs cleaning Severity set to `wishlist'.> thanksStopping processing here. Please contact me if you need assistance. Debian bug tracking system administrator (administrator, Debian Bugs database)
Debian Bug Tracking System
2004-Sep-20 21:48 UTC
[Logcheck-devel] Bug#272047: marked as done (logcheck's greplogoutput code needs cleaning)
Your message dated Mon, 20 Sep 2004 23:45:05 +0200 with message-id <20040920214505.GC1731 at v216-190.vps.tuwien.ac.at> and subject line Bug#272047: [Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning has caused the attached Bug report to be marked as done. This means that you claim that the problem has been dealt with. If this is not the case it is now your responsibility to reopen the Bug report if necessary, and/or fix the problem forthwith. (NB: If you are a system administrator and have no idea what I am talking about this indicates a serious mail system misconfiguration somewhere. Please contact me immediately.) Debian bug tracking system administrator (administrator, Debian Bugs database) -------------------------------------- Received: (at submit) by bugs.debian.org; 17 Sep 2004 05:58:59 +0000>From RossBoylan at stanfordalumni.org Thu Sep 16 22:58:59 2004Return-path: <RossBoylan at stanfordalumni.org> Received: from asmtp-a063f33.pas.sa.earthlink.net [207.217.120.149] by spohr.debian.org with esmtp (Exim 3.35 1 (Debian)) id 1C8Blr-0002ax-00; Thu, 16 Sep 2004 22:58:59 -0700 Received: from dialup-4.243.206.155.dial1.sanfrancisco1.level3.net ([4.243.206.155] helo=wheat.dslnorthwest.net) by asmtp-a063f33.pas.sa.earthlink.net with asmtp (Exim 4.34) id 1C8Blp-0008Pv-KV; Thu, 16 Sep 2004 22:58:58 -0700 Received: from ross by wheat.dslnorthwest.net with local (Exim 3.36 #1 (Debian)) id 1C8Bln-0003xy-00; Thu, 16 Sep 2004 22:58:55 -0700 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Ross Boylan <RossBoylan at stanfordalumni.org> To: Debian Bug Tracking System <submit at bugs.debian.org> CC: RossBoylan at stanfordalumni.org Subject: logcheck's greplogoutput code needs cleaning X-Mailer: reportbug 2.63 Date: Thu, 16 Sep 2004 22:58:55 -0700 Message-Id: <E1C8Bln-0003xy-00 at wheat.dslnorthwest.net> Sender: Ross Boylan <RossBoylan at stanfordalumni.org> X-ELNK-Trace: 9993cece113c45e785338a7d01cb3b6a7e972de0d01da94010c1fd88c21225838bc591ae6838bfd2350badd9bab72f9c350badd9bab72f9c350badd9bab72f9c X-Originating-IP: 4.243.206.155 Delivered-To: submit at bugs.debian.org X-Spam-Checker-Version: SpamAssassin 2.60-bugs.debian.org_2004_03_25 (1.212-2003-09-23-exp) on spohr.debian.org X-Spam-Status: No, hits=-7.0 required=4.0 tests=BAYES_01,HAS_PACKAGE autolearn=no version=2.60-bugs.debian.org_2004_03_25 X-Spam-Level: Package: logcheck Version: 1.2.25 Severity: normal Unless my understanding of shell scripts is off, there seem to be quite a few problems with the greplogoutput routine. First, individual sections of the code contain comments that clearly do not match what the code is doing. Second, the overall sequencing and flow of the tests (as written--perhaps less so as commented) doesn't make sense. Consider # Now ignore all entries from the logcheck-<package> files if [ -n "$ignore" ]; then debug "Applying Logcheck override files" for file in $(ls -1 $ignore/) ; do debug "clean logcheck-<package>: $file" cleanchecked "$ignore/$file" done else The code is actually using every single ignore file, though the comments say it should be using only the logcheck-* files. Given that, the subsequent efforts to pick out various subsets of the ignore files are completely redundant. Now even if only logcheck-* were run, the subsequent section debug "Cleaning logcheck" # Remove any entries already reported for file in $(ls $raise/ | grep -v '^logcheck') ; do debug "Cleaning logcheck: $file" cleanchecked "$raise/$file" done doesn't make much sense. The very first action of greplogoutput is to look for an ignore file of the same name, which in this case must be logcheck. The previous code, if it followed the comments, would have got all the logcheck-* files. So this section redoes all those things that have already been done. It's only "contribution" is to pick up logcheck* files that aren't logcheck-*, and it's not clear to me that's desirable (or that there are likely to be any such files). Finally, the overall logic according to the comments seems unnecessarily complex. For every package foo, the ignore files named foo logcheck-foo local local-* are ignored. Why not local-foo instead of local-*? Then there is the added wrinkle of the separate handling when the "raise" file name is logcheck. I encourage you to decide what you want this code to do and then make the code and the comments match that behavior. Finally, *please* expand the manual page to document what the behavior is. My patch in bug #215640 provides such a revised man page. I believe it reflected the actual behavior of the code at one point; there may have been changes since, and it will need to reflect whatever decisions are made dealing with the issues raised above. I really would like to avoid the necessity of rereading the code every time I want to figure out how to fill in the configuration files :) -- System Information: Debian Release: 3.1 APT prefers testing APT policy: (990, 'testing'), (50, 'unstable') Architecture: i386 (i686) Kernel: Linux 2.4.26advncdfs Locale: LANG=en_US, LC_CTYPE=en_US Versions of packages logcheck depends on: ii adduser 3.59 Add and remove users and groups ii cron 3.0pl1-86 management of regular background p ii debconf [debconf 1.4.30.3 Debian configuration management sy ii debianutils 2.8.4 Miscellaneous utilities specific t ii exim [mail-trans 3.36-11 An MTA (Mail Transport Agent) ii lockfile-progs 0.1.10 Programs for locking and unlocking ii logcheck-databas 1.2.25 A database of system log rules for ii logtail 1.2.25 Print log file lines that have not ii mailx 1:8.1.2-0.20040524cvs-1 A simple mail user agent ii perl 5.8.4-2 Larry Wall's Practical Extraction ii sysklogd [system 1.4.1-15 System Logging Daemon -- debconf information: * logcheck/security_level: workstation * logcheck/manage_conffiles: true * logcheck/auto_create_logfiles: true logcheck/upgrade-note: * logcheck/changes: * logcheck/install-note: * logcheck/rules-directories-note: * logcheck/email_address: root * logcheck/rewrite-note: --------------------------------------- Received: (at 272047-done) by bugs.debian.org; 20 Sep 2004 21:44:59 +0000>From max at v216-190.vps.tuwien.ac.at Mon Sep 20 14:44:59 2004Return-path: <max at v216-190.vps.tuwien.ac.at> Received: from baikonur.stro.at [213.239.196.228] by spohr.debian.org with esmtp (Exim 3.35 1 (Debian)) id 1C9Vxz-0002AF-00; Mon, 20 Sep 2004 14:44:59 -0700 Received: from localhost (localhost [127.0.0.1]) by baikonur.stro.at (Postfix) with ESMTP id E2DD95C06C for <272047-done at bugs.debian.org>; Mon, 20 Sep 2004 23:44:54 +0200 (CEST) Received: from baikonur.stro.at ([127.0.0.1]) by localhost (baikonur [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 29375-09 for <272047-done at bugs.debian.org>; Mon, 20 Sep 2004 23:44:54 +0200 (CEST) Received: from sputnik (v216-190.vps.tuwien.ac.at [128.131.216.190]) by baikonur.stro.at (Postfix) with ESMTP id 7102C5C034 for <272047-done at bugs.debian.org>; Mon, 20 Sep 2004 23:44:54 +0200 (CEST) Received: from max by sputnik with local (Exim 4.34) id 1C9Vy5-00069G-Ha for 272047-done at bugs.debian.org; Mon, 20 Sep 2004 23:45:05 +0200 Date: Mon, 20 Sep 2004 23:45:05 +0200 From: maks attems <debian at sternwelten.at> To: 272047-done at bugs.debian.org Subject: Re: Bug#272047: [Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning Message-ID: <20040920214505.GC1731 at v216-190.vps.tuwien.ac.at> References: <E1C8Bln-0003xy-00 at wheat.dslnorthwest.net> <20040918211750.GD9492 at stro.at> <20040919041135.GK3198 at wheat.boylan.org> <20040919090124.GA1731 at v216-190.vps.tuwien.ac.at> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040919090124.GA1731 at v216-190.vps.tuwien.ac.at> User-Agent: Mutt/1.5.6+20040722i Sender: maximilian attems <max at v216-190.vps.tuwien.ac.at> X-Virus-Scanned: by Amavis (ClamAV) at stro.at Delivered-To: 272047-done at bugs.debian.org X-Spam-Checker-Version: SpamAssassin 2.60-bugs.debian.org_2004_03_25 (1.212-2003-09-23-exp) on spohr.debian.org X-Spam-Status: No, hits=-5.0 required=4.0 tests=BAYES_01,HAS_BUG_NUMBER autolearn=no version=2.60-bugs.debian.org_2004_03_25 X-Spam-Level: nothing conclusive come out of that bug. the debian logcheck team knows this weakness. the named concerns do not limit logcheck functionality. they were added to the logcheck TODO in cvs for next release.